-
Notifications
You must be signed in to change notification settings - Fork 189
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use ocis registry for refresh #2120
Conversation
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please consider a more speaking variable name for r
such as ocisRegistry.
Yes, I am very well aware. But here we have a different case. If you look at the rule of thumb from the first link you sent (which is A name's length should not exceed its information content.) this one is mainly true for local 'helper' variables that do not have a deep intersection with the code around. But that is not the case in this PR: Here we have two different registries that can be confused. Your whole bugfix is about choosing the right one - but with shortening the var name that drastically, I think the confusion for the next reader of this code is pre set. Thus my ask to make it somehow visible that this is about the oCIS registry. I have to admit that in times of auto completion I am not so much convinced of the the short-name convention that is described in the links you referenced. But ok. However, in this case, I am even convinced that the rules they make do not even apply. |
1ff8c00
to
7629162
Compare
@IljaN please rebase, Settings UI tests were fixed in master. |
The go-micro registry-singleton ignores the ocis configuration and defaults to mdns
7629162
to
14de925
Compare
Kudos, SonarCloud Quality Gate passed! |
Description
The go-micro registry-singleton ignores the ocis config and defaults to mdns
Related Issue
Motivation and Context
The storage was always advertised via mdns, ignoring the registry configuration
Types of changes
Checklist: