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

Prepend arg for defaults-file #408

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nr-dbuckwalter
Copy link

Overview

The mysql CLI depends on the --defaults-file=foo argument being the very first argument. This change provides a new 'PrependArgs' method and uses it for the mysql plugin.

From the mysql CLI help text:

The following groups are read: mysql client
The following options may be given as the first argument:
--print-defaults        Print the program argument list and exit.
--no-defaults           Don't read default options from any option file,
                        except for login file.
--defaults-file=#       Only read default options from the given file #.
--defaults-extra-file=# Read this file after the global files are read.
--defaults-group-suffix=#
                        Also read groups with concat(group, suffix)
--login-path=#          Read this path from the login file.

Type of change

  • Created a new plugin
  • Improved an existing plugin
  • Fixed a bug in an existing plugin
  • Improved contributor utilities or experience

Related Issue(s)

How To Test

Build the plugin locally and run op plugin run mysql -- --host=foo using a credential with no host field. 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=foo as MySQL configuration variable. After the PR, additional command-line arguments are appended after --defaults-file that is injected by the mysql plugin.

Changelog

  • The mysql CLI depends on the --defaults-file=foo argument being the very first argument

Copy link
Contributor

⚠️ This PR contains unsigned commits. To get your PR merged, please sign those commits (git commit -S --amend --no-edit) and force push them to this branch (git push --force-with-lease).

If you're new to commit signing, there are different ways to set it up:

Sign commits with gpg

Follow the steps below to set up commit signing with gpg:

  1. Generate a GPG key
  2. Add the GPG key to your GitHub account
  3. Configure git to use your GPG key for commit signing
Sign commits with ssh-agent

Follow the steps below to set up commit signing with ssh-agent:

  1. Generate an SSH key and add it to ssh-agent
  2. Add the SSH key to your GitHub account
  3. Configure git to use your SSH key for commit signing
Sign commits with 1Password

You can also sign commits using 1Password, which lets you sign commits with biometrics without the signing key leaving the local 1Password process.

Learn how to use 1Password to sign your commits.

Watch the demo

@nr-dbuckwalter
Copy link
Author

Added my GPG key to Github -- commits are signed

Copy link
Contributor

@AndyTitu AndyTitu left a comment

Choose a reason for hiding this comment

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

lgtm

@minaguib
Copy link

minaguib commented Feb 8, 2024

Any chance to get this approved and released ? The plugin is otherwise broken for all but the simplest usecases.

@nr-dbuckwalter
Copy link
Author

@AndyTitu can you help with getting a second approver? Any chance you could add a CODEOWNERS or some other mechanism to help find the right reviewers in the future?

Copy link

@francoisjacques francoisjacques left a 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.

@minaguib
Copy link

Any maintainers able to merge to get this officially into next 1password-cli release ?

@francoisjacques
Copy link

@SimonBarendse this seems ready to go. Can you provide guidance if there are any steps missing?

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.

1Password mysql shell plugin breaks additional CLI arguments
4 participants