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

Normalize plugin_name option for mount and enable-auth #3202

Merged
merged 4 commits into from
Aug 31, 2017
Merged

Conversation

calvn
Copy link
Contributor

@calvn calvn commented Aug 17, 2017

Fixes #3180

@calvn calvn added this to the next-release milestone Aug 17, 2017
@jefferai jefferai modified the milestones: next-release, 0.8.2 Aug 18, 2017
PluginName string `json:"plugin_name,omitempty" structs:"plugin_name,omitempty"`
}

type AuthConfigInput struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are arguments against it (mostly being, maybe what we need will diverge in the future) but it seems like simply reusing MountConfigInput could be good here. This allows the same structure to be used for tuning both (ttl, maxttl, updated plugin name, etc.).

Copy link
Contributor Author

@calvn calvn Aug 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is ForceNoCache a property of auth mounts as well? Can the same be said for MountConfigOutput and AuthConfigOutput?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is. Most mounts do nothing with it, but it is technically able to be set. So -- yes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about the second question, should we use MountConfigOutput for both cases then since the config params should be the same?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that this would be an API change. But -- then we're inconsistent because you get an AuthConfigOutput with a MountConfigInput whereas for secrets you get MountConfigInput/MountConfigOutput.

Part of me says "this is a silly situation to be in already and let's just break it and be done with it" but...we probably shouldn't. So I guess, leave AuthConfigInput after all.

@jefferai jefferai merged commit d10075e into master Aug 31, 2017
@jefferai jefferai deleted the plugin-config branch August 31, 2017 16:17
chrishoffman pushed a commit that referenced this pull request Sep 1, 2017
* oss/master:
  Plugin Version Update (#3275)
  Lazy-load plugin mounts (#3255)
  changelog++
  changelog++
  Add pki/root/sign-self-issued. (#3274)
  Travis, be happier please
  changelog++
  Change auth helper interface to api.Secret. (#3263)
  changelog++
  Try reconnecting Mongo on EOF (#3269)
  Don't append a trailing slash to the request path if it doesn't actually help find something (#3271)
  changelog++
  Use TypeDurationSecond for TTL values in PKI. (#3270)
  changelog++
  changelog++
  Use net.SplitHostPort on Consul address (#3268)
  Normalize plugin_name option for mount and enable-auth (#3202)
  Updating Okta lib for credential backend (#3245)
  Explicitly mention that aws/aws-ec2 were unified under aws.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants