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

Add support for MongoDB Atlas CLI, with environment variable based importing and provisioning #198

Merged
merged 5 commits into from Jun 1, 2023

Conversation

joqim
Copy link
Contributor

@joqim joqim commented Mar 13, 2023

This PR adds support for the MongoDB Atlas CLI. This is different from the core MongoDB CLI.

Core features

  • The authentication setup currently is "Public Key" and "Private Key". These are obtained from the Altas organization > Access Manager > API Keys. (One part that's not clear to me yet is, does the key generated here apply to all the projects within the organization? I am failing to a project-specific API generator)
  • The shell plugin looks for the Atlas CLI's environment variables and imports into 1Password.
  • If there are no environment variables in the current shell session, 1Password prompts the user to manually enter them.
  • Provisions the shell plugin for every "atlas" command (except help and version commands) with the environment variables.

Main issues

  • Atlas CLI supports config file too, but we don't look for it and import the credentials just yet because it uses a different type of authentication: access token, and refresh token.
  • When there is no Atlas CLI config file, but if a MongoDB CLI config file exists, the Atlas commands show a prompt asking if the user would like to import the config file from MongoDB CLI path to the Atlas CLI path. This kind of hinders the 1Password shell plugin's authentication flow.

This is my first time contributing to Shell Plugins, feedback welcome. Thanks, @arunsathiya, for getting me started with 1Password and Shell Plugins.

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.

Code looks good to me overall, I just left a few nitpicks. Awesome job @joqim , thanks for contributing!

plugins/atlas/atlas.go Outdated Show resolved Hide resolved
plugins/atlas/atlas.go Outdated Show resolved Hide resolved
plugins/atlas/credentials.go Outdated Show resolved Hide resolved
plugins/atlas/plugin.go Outdated Show resolved Hide resolved
plugins/atlas/credentials_test.go Outdated Show resolved Hide resolved
@AndyTitu
Copy link
Contributor

I think supporting the environment variables - public/private key-pair - workflow for now is a good approach. We could introduce the config file auth flow further down the line.

Copy link
Contributor

@arunsathiya arunsathiya left a comment

Choose a reason for hiding this comment

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

Great to see your first PR, Joqim! 🎉

Overall, it looks good to me but I left one comment here to discuss further about the credential name.

@joqim
Copy link
Contributor Author

joqim commented Mar 17, 2023

Thank you for your feedback. I'll address these recommendations as soon as possible.

@joqim
Copy link
Contributor Author

joqim commented Apr 9, 2023

I've addressed your changes, please take a look into it.

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

Copy link
Contributor

@arunsathiya arunsathiya left a comment

Choose a reason for hiding this comment

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

The current changes look good to me too.

plugins/atlas/apikey.go Outdated Show resolved Hide resolved
Copy link
Member

@SimonBarendse SimonBarendse left a comment

Choose a reason for hiding this comment

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

Appreciate the contribution! Thank you for taking the time to submit ❤️

…ntial

Co-authored-by: Floris van der Grinten <floris@grinten.com>
Co-authored-by: Simon Barendse <SimonBarendse@users.noreply.github.com>
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.

re-approving

@AndyTitu AndyTitu merged commit c95bf76 into 1Password:main Jun 1, 2023
2 checks passed
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.

None yet

6 participants