-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Support version selection for database plugins #16982
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.
Looks good to me. I haven't had the chance to take it for a spin yet, but that shouldn't hold up the merge. Nor should the nits in my review.
@@ -306,7 +306,7 @@ const mountStateUnmounting = "unmounting" | |||
type MountEntry struct { | |||
Table string `json:"table"` // The table it belongs to | |||
Path string `json:"path"` // Mount Path | |||
Type string `json:"type"` // Logical backend Type | |||
Type string `json:"type"` // Logical backend Type. NB: This is the plugin name, e.g. my-vault-plugin, NOT plugin type (e.g. auth). |
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.
+1 this is a confusing overloading of terms. A bit of clarity is good.
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.
So far, looks great.
Thanks for the reviews so far - I think it's ready for another pass @swenson |
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
Supports specifying a
plugin_version
when configuring a database plugin.To test locally: