Skip to content
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

Merged
merged 1 commit into from
Jun 7, 2021
Merged

Use ocis registry for refresh #2120

merged 1 commit into from
Jun 7, 2021

Conversation

IljaN
Copy link
Member

@IljaN IljaN commented Jun 5, 2021

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

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

@IljaN IljaN added the Type:Bug label Jun 5, 2021
@IljaN IljaN requested review from butonic, C0rby and refs June 5, 2021 19:10
@IljaN IljaN self-assigned this Jun 5, 2021
@update-docs
Copy link

update-docs bot commented Jun 5, 2021

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.

Copy link
Contributor

@dragotin dragotin left a 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.

@IljaN
Copy link
Member Author

IljaN commented Jun 7, 2021

@IljaN IljaN requested a review from dragotin June 7, 2021 08:50
@dragotin
Copy link
Contributor

dragotin commented Jun 7, 2021

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.

@micbar
Copy link
Contributor

micbar commented Jun 7, 2021

@IljaN please rebase, Settings UI tests were fixed in master.

The go-micro registry-singleton ignores the ocis configuration and defaults to mdns
@sonarcloud
Copy link

sonarcloud bot commented Jun 7, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@IljaN IljaN merged commit 073503b into master Jun 7, 2021
@delete-merged-branch delete-merged-branch bot deleted the use-configured-registry branch June 7, 2021 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Legacy storage registration ignores config
4 participants