A bit of code refactoring #805
Conversation
…s if the length of the registries is > 0 which we're doing after that and is not being used anywhere else
There was a problem hiding this comment.
Thanks for this contribution @EloyTolosaDev! One requested change here
pkg/autosync/autosync.go
Outdated
|
|
||
| registries, err := registry.GetProfileRegistries(interactive) | ||
| if err != nil { | ||
| clio.Debugf("unable to load granted config file with err %s", err.Error()) | ||
| return | ||
| } | ||
|
|
||
| if len(registries) > 0 { |
There was a problem hiding this comment.
registry.IsOutdatedConfig() checks a different field in the Granted config to the one in registry.GetProfileRegistries().
IsOutdatedConfig checks if the length of ProfileRegistryURLS is > 0, whereas GetProfileRegistries uses the ProfileRegistry field.
The change you have made here will cause valid profile registry configuration to return the error. Could you please restore the previous behaviour?
In this instance, I'd propose just restoring the registry.IsOutdatedConfig function and restoring the call to the function. The code there was working in production, so there is no need to change it IMO.
|
Hi! Thanks for the response. I checked what you said and you're totally true, I was confused about profileRegistryURLs field and the actual loadedRegistry structs. Kind regards |
It started because I wanted to investigate on issue #785 , and I ended up refactoring the codebase a bit while working on it. In the end, I did nothing about the issue because I couldn't figure out what was happening, and I did not get any response from the person who created the issue, so I could not investigate further. After all, it looks like a user problem, as I was unable to reproduce the problem.
What changed?
Functionality did not change, the main changes were:
Why?
How did you test it?
I tested the dassume and dgranted binaries and worked as espected.
Potential risks
Is patch release candidate?
Link to relevant docs PRs