-
Notifications
You must be signed in to change notification settings - Fork 244
Prepend arg for defaults-file #408
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
base: main
Are you sure you want to change the base?
Conversation
|
If you're new to commit signing, there are different ways to set it up: Sign commits with
|
|
Added my GPG key to Github -- commits are signed |
46ce3d1 to
e45e42f
Compare
AndyTitu
left a comment
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
|
Any chance to get this approved and released ? The plugin is otherwise broken for all but the simplest usecases. |
|
@AndyTitu can you help with getting a second approver? Any chance you could add a |
francoisjacques
left a comment
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.
Prepending appears a reasonable and pragmatic approach.
|
Any maintainers able to merge to get this officially into next 1password-cli release ? |
|
@SimonBarendse this seems ready to go. Can you provide guidance if there are any steps missing? |
|
@mrjones2014 I see you are a recent committer/merger -- can you approve and merge this please? |
| } | ||
|
|
||
| out.AddArgs(argsResolved...) | ||
| out.PrependArgs(argsResolved...) |
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.
Do we want to do this unconditionally or just for mysql? We should verify this doesn't break any of the existing plugins first.
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.
Fair enough, though it's kind of unique to the mysql CLI that flag-type arguments be order-sensitive.
It's been a long while since I made this PR but as I recall, scoping the change to a single plugin was more involved. I will do what I can but may not have any time in the near future to fix up my PR limit the argument ordering change to a single plugin.
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.
It may be fine as is. We just need to verify it doesn’t break anything.
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.
I wanted to surface that this PR extends the work here and addresses @mrjones2014 concerns
Overview
The
mysqlCLI depends on the--defaults-file=fooargument being the very first argument. This change provides a new 'PrependArgs' method and uses it for themysqlplugin.From the
mysqlCLI help text:Type of change
Related Issue(s)
How To Test
Build the plugin locally and run
op plugin run mysql -- --host=foousing a credential with nohostfield. Before this PR, it results in the error:mysql: [ERROR] unknown variable 'defaults-file=...because the MySQL treats unknown arguments in the form--variable-name=fooas MySQL configuration variable. After the PR, additional command-line arguments are appended after--defaults-filethat is injected by themysqlplugin.Changelog
mysqlCLI depends on the--defaults-file=fooargument being the very first argument