-
Notifications
You must be signed in to change notification settings - Fork 192
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
Respect no_prefix for Consul keys #204
Conversation
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.
LGTM!
This changes the default behavior for Consul keys. Currently they default to having no prefix, as if no_prefix defaulted to true. This changes no_prefix's default to false, so that they would all have prefixes. I'm afraid changing the default behavior will break existing uses. Would it be possible to have it default to true? |
We could have Right now they default to including the prefix when Thoughts, @banks? |
I think inconsistency is not nice but it's better than breaking changes!
I'd vote for keeping Consul's default behaviour unchanged even if that
means this argument has a different default for Consul and Vault - ideally
we could call that out in docs (godoc and readme for now but I think later
we'll want something richer?)
I'd defer to John on that though. Great catch John!
…On Tue, May 14, 2019 at 12:09 AM Freddy ***@***.***> wrote:
We could have NoPrefix default to true, but then we introduce a similar
issue for Vault keys.
Right now they default to including the prefix when no_prefix isn't set.
Thoughts, @banks <https://github.com/banks>?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#204?email_source=notifications&email_token=AAA5QU6YWLFYN6MWKHU7HMDPVHYKXA5CNFSM4HJE5J72YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODVJZZNQ#issuecomment-492018870>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAA5QU6F6ALMQR3AVRCCIZ3PVHYKXANCNFSM4HJE5J7Q>
.
|
@eikenb I took a pass at allowing Consul and Vault to have different defaults for Now users who weren't setting that flag won't see a change in behavior. |
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.
The new changes look good. I think we'll revisit this issue in the future to see if we can get the defaults in sync... but for now I like this solution.
Fixes: #190
Previously, the configuration option
no_prefix
was implemented for keys read from Vault but not from Consul.This PR implements the behavior as stated in the docs: