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

fix: Improve the UI for the influx v1 auth commands #19935

Merged
merged 4 commits into from
Nov 9, 2020
Merged

Conversation

stuartcarnie
Copy link
Contributor

@stuartcarnie stuartcarnie commented Nov 9, 2020

Closes #19933

This PR improves the v1 auth commands of the influx CLI tool, and includes some minor improvements to the kit/cli package. The enhancements and fixes to the user experience are as follows:

  • Rename --auth-token to --username for the v1 auth create command to clarify its purpose in the context of v1 requests

  • v1 auth create now requires a password when creating a new authorisation. It can be optionally disabled with the --no-password flag, however, the user much set a password using the v1 auth set-password command prior to using the token.

  • Rename v1 auth active and v1 auth inactive commands to the actions set-active and set-inactive respectively to clarify their behaviour.

    For example, to set a token to active:

    influx v1 auth set-active (--id|--username)
    

    To set a token to inactive, preventing its use in subsequent requests:

    influx v1 auth set-inactive (--id|--username)
    
  • Universally allow the --id or --username flags anywhere a single authentication token is
    required

  • Use separate global variables to store the flag state of each v1 auth command

  • factor out the behavior to find a single authentication token into
    a shared function, v1FindOneAuthorization

The enhancements and fixes to the user experience are as follows:

* Rename active and inactive commands to the actions set-active and
  set-inactive respectively to clarify their behavior

  To set a token to active:

  ```
  influx v1 auth set-active
  ```

  To set a token to inactive, preventing its use:

  ```
  influx v1 auth set-inactive
  ```

* allow `--id` or `--username` anywhere a single authentication token is
  required

* use separate vars to store the flag state of each command

* factor out the behavior to find a single authentication token into
  a shared function, `v1FindOneAuthorization`
@stuartcarnie stuartcarnie added the area/2.x OSS 2.0 related issues and PRs label Nov 9, 2020
@stuartcarnie stuartcarnie self-assigned this Nov 9, 2020
cmd/influx/v1_authorization.go Show resolved Hide resolved
@@ -479,9 +519,78 @@ func v1AuthorizationInactiveF(cmd *cobra.Command, args []string) error {
})
}

var (
errMultipleMarchingAuthorizations = errors.New("multiple authorizations found")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
errMultipleMarchingAuthorizations = errors.New("multiple authorizations found")
errMultipleMatchingAuthorizations = errors.New("multiple authorizations found")

Copy link
Contributor Author

@stuartcarnie stuartcarnie Nov 9, 2020

Choose a reason for hiding this comment

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

Thanks! I am not used to typing on this laptop keyboard – one of the joys of being in quarantine in Australia right now, and not having access to my external keyboard 🤣

func (f *v1AuthLookupFlags) validate() error {
switch {
case f.id.Valid() && f.username != "":
return errors.New("specify id or username")
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we reword the error to make it more clear that it's an XOR? At first glance I wasn't sure why this was an error.

Copy link
Contributor Author

@stuartcarnie stuartcarnie Nov 9, 2020

Choose a reason for hiding this comment

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

How about

specify id or username, not both

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@danxmoran danxmoran left a comment

Choose a reason for hiding this comment

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

:shipit: assuming CI passes

@danxmoran danxmoran merged commit 85b9d84 into master Nov 9, 2020
@stuartcarnie stuartcarnie deleted the sgc/issues/19933 branch November 9, 2020 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/2.x OSS 2.0 related issues and PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve UI for v1 auth create CLI command
2 participants