Pylon auth update#7
Conversation
jakub-zytka-reef
left a comment
jakub-zytka-reef
left a comment
There was a problem hiding this comment.
oh shit, I added comments but haven't published the review.
Sorry, my bad
| } | ||
| if *netUID >= 0 { | ||
| config.SetNetUID(*netUID) | ||
| } else if config.GetNetUID() < 0 { |
There was a problem hiding this comment.
nit: why else?
I just want to understand your way of thinking
There was a problem hiding this comment.
in configuration.NewConfig() netUID is set based on the NEXUS_PYLON_NETUID env variable or it's set to -1. So here the else block is making sure a valid value is set for that env variable in case the user doesn't specify the netUID when calling the run command.
There was a problem hiding this comment.
OK, so when I read:
} else if config.GetNetUID() < 0 {
// netuid is required
then I think - OK, netuid is required, but not always. Only when the previous condition evaluates to false so that the else triggers.
So I've got to go back and check when is netuid not required when the condition evaluates to true.
It's too complex and I'm lazy.
Thus I write just
if config.GetNetUID() < 0 {
// netuid is required
}
and then I know that netuid must be provided always, regardless of whether it comes from the env, or the cli.
So that's why I asked if there's a reason for having else.
There was a problem hiding this comment.
That makes sense. That way is easier to read as well. I'll update it accordingly.
| if cachedKey, found := a.cache.Get(sanitizedOrgName); found { | ||
| log.Printf("Cache hit for organization '%s'", sanitizedOrgName) | ||
| return cachedKey, nil | ||
| if err := a.validatePublicKey(cert, cachedKey); err != nil { |
There was a problem hiding this comment.
IIUC, as discussed on slack, we don't need these caching-related changes?
There was a problem hiding this comment.
I think the conclusion we came to was to only refresh the cache every 3 minutes. Should I implement that? Or just remove my cache invalidation logic?
There was a problem hiding this comment.
I think your cache invalidation logic should be removed.
In order to have 3 min TTL for the cache entries - is there anything specific that needs to be implemented? I don't remember, but I think we already have TTL in the cache?
There was a problem hiding this comment.
you're right. I'll remove my cache invalidation.
2 participants
Update nexus auth to support: