-
Notifications
You must be signed in to change notification settings - Fork 184
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
Implement methods on Registry
for managing prefix and common labels
#400
base: master
Are you sure you want to change the base?
Conversation
} | ||
|
||
/// `labels` returns a copy of the common labels for the registry, if any. | ||
pub fn labels(&self) -> Option<HashMap<String, String>> { |
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.
Here and in prefix()
above, can we return a reference instead (and let the consumers clone if they want)?
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.
No, because the borrow would outlive the function call.
Thanks for the PR! In order to merge, this will need a rebase and a I think we'd need a bit more context behind this. I do like the getters (minus the clone part), but I'm not thrilled by the set/clean logic. I won't expect people to use the registry that way, and the fact that those methods internally lock will likely become a bad performance surprise for many people. |
I think the problem this PR is inspired by is that it's Disclaimer: I'm not the author, they may have another use case. |
@polachok thanks, that was useful feedback! Next release should come with #396 which should hopefully improve that UX aspect, but taking a different path from this PR. That said, I'm still intimately not in favor of doing this kind of registry mutation at runtime. It's a bazooka with side-effects at distance, which can easily turn into a very large footgun. |
Right, I think based on your feedback that I wasn't clear on the use-case. Specifically, this is intended to make it possible to manipulate the set of default labels on the default registry. I do not believe that this adds any useful functionality for custom registries. However, I think custom registries are probably a very niche use-case anyway:
Because of this, I always use the default registry, and I bet 99% of your users do too.
Keeping getters and removing setters doesn't add any meaningful functionality imo. The functionality I'm trying to add here is the ability to set default labels on the default registry.
Again, this is only for default labels on a Registry. Presumably default labels would only need to be configured once, not continuously. |
This allows users to set the registry's prefix or common labels when using the default registry. Signed-off-by: Alex Forster <[email protected]>
I've rebased and added a |
I thought I was going to need this feature, and was about to use this branch via a I do have a use case for multiple registries though -- we have an application that we deliver to users, and also operate as a hosted service. Some metrics measure implementation details or unstable features, so we initially add them to the default registry (which is only exposed in internal builds) and later switch them to a different registry when we're sure that they're useful for users and unlikely to be changed. |
This allows users to set the registry's prefix or common labels when using the default registry.