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

feat(cmd/influx): add secret cli #16786

Merged
merged 2 commits into from
Feb 10, 2020
Merged

feat(cmd/influx): add secret cli #16786

merged 2 commits into from
Feb 10, 2020

Conversation

kelwang
Copy link
Contributor

@kelwang kelwang commented Feb 7, 2020

Closes #16475

Add the http secret service, cli, and tests

  • CHANGELOG.md updated with a link to the PR (not the Issue)
  • Well-formatted commit messages
  • Rebased/mergeable
  • Tests pass
  • http/swagger.yml updated (if modified Go structs or API)
  • Documentation updated or issue created (provide link to issue/pr)
  • Signed CLA (if not already signed)

@kelwang kelwang changed the title feat cli secret feat(cmd/influx): add secret cli Feb 7, 2020
@kelwang kelwang requested a review from a team February 7, 2020 21:05
cmd/influx/secret.go Outdated Show resolved Hide resolved
Copy link
Contributor

@jsteenb2 jsteenb2 left a comment

Choose a reason for hiding this comment

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

the only must chagne is fixing the error to match up with the domain

cmd/influx/secret.go Outdated Show resolved Hide resolved
cmd/influx/secret.go Show resolved Hide resolved
cmd/influx/setup.go Outdated Show resolved Hide resolved
cmd/influx/setup.go Show resolved Hide resolved
http/org_service.go Show resolved Hide resolved
http/org_service.go Show resolved Hide resolved
http/org_service.go Show resolved Hide resolved
http/org_service.go Show resolved Hide resolved
http/org_service_test.go Show resolved Hide resolved
Copy link
Contributor

@jsteenb2 jsteenb2 left a comment

Choose a reason for hiding this comment

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

here we go

@kelwang kelwang merged commit 2761c1f into master Feb 10, 2020
@kelwang kelwang deleted the feat_cli_secret branch February 10, 2020 17:39
Comment on lines +57 to +61
cmd := b.newCmd("update", b.cmdUpdateRunEFn)
cmd.Short = "Update secret"
cmd.Flags().StringVarP(&b.key, "key", "k", "", "The secret key (required)")
cmd.MarkFlagRequired("key")
b.org.register(cmd, false)
Copy link
Contributor

Choose a reason for hiding this comment

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

You can pass a key, but how do you update the value of the secret?

Copy link
Contributor

Choose a reason for hiding this comment

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

@sanderson This is done through a prompt similar to our password flow. I'm adding a card to provide it over the CLI as an arg in the same command.

Comment on lines +139 to +141
cmd := b.newCmd("find", b.cmdFindRunEFn)
cmd.Short = "Find secrets"
b.org.register(cmd, false)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an overall critique of "find"/"list" commands throughout the influx CLI. We are inconsistent. For some it's "list" and others it's "find". The term "find" indicates a searching action and I would expect ways to filter the results. This just lists all the secret keys.

My personal preference would be to make everything consistent and use either list or ls to align with standard Linux/UNIX command line tools.

@jademcgough

Copy link
Contributor

Choose a reason for hiding this comment

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

@sanderson, this x1000! I've had the same feelings towards list|find. I would really love to see it become list if it does not allow for filtering. We do have filtering on some Find commands, either by name or what not. But these are still akin to list operations for me in most nix envs. I would love to see list and ls as an alias shorthand for calling it 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

@sanderson we're going to make these changes 👍. @influxdata/tools-team is on board 🤘

Copy link
Contributor

Choose a reason for hiding this comment

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

@jsteenb2 🎉

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.

Add the ability to manage secrets from the CLI
4 participants