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

Refactor apps [CLI-15] #111

Merged
merged 8 commits into from
Feb 26, 2021
Merged

Refactor apps [CLI-15] #111

merged 8 commits into from
Feb 26, 2021

Conversation

Widcket
Copy link
Contributor

@Widcket Widcket commented Feb 26, 2021

Description

  • Used the common prompt helpers and removed references to the underlying prompts library
  • Renamed clients.go to apps.go
  • Added clients as an alias
  • Added a confirmation prompt to apps delete
  • Added apps update so the basic CRUD is now complete
  • Cleaned up the text and references to clients

apps create
https://user-images.githubusercontent.com/5055789/109328681-7ae10100-7838-11eb-8507-ced24545b6ac.mov

apps update
https://user-images.githubusercontent.com/5055789/109328732-8cc2a400-7838-11eb-8380-2ef47bd8dd9c.mov

apps delete
https://user-images.githubusercontent.com/5055789/109328803-a106a100-7838-11eb-88e3-4e691b4ed04e.mov

Testing

  • This change adds test coverage for new/changed/fixed functionality

Checklist

  • I have added documentation for new/changed functionality in this PR or in auth0.com/docs
  • All active GitHub checks for tests, formatting, and security are passing
  • The correct base branch is being used, if not master

@@ -265,7 +265,7 @@ auth0 apis update --id id --name myapi
cmd.Flags().StringVarP(&flags.ID, apiID, "i", "", "ID of the API.")
cmd.Flags().StringVarP(&flags.Name, apiName, "n", "", "Name of the API.")
cmd.Flags().StringVarP(&flags.Scopes, apiScopes, "s", "", "Space-separated list of scopes.")
mustRequireFlags(cmd, apiID, apiName)
mustRequireFlags(cmd, apiID)
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: we don't need apiName because it's optional for creation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's optional for update (you can update any property). For create it's required.

@Widcket Widcket requested a review from cyx February 26, 2021 18:55
Comment on lines -303 to -311
err := ansi.Spinner("Deleting API", func() error {
return ansi.Spinner("Deleting API", func() error {
return cli.api.ResourceServer.Delete(flags.ID)
})

if err != nil {
return err
}

return nil
Copy link
Contributor

Choose a reason for hiding this comment

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

#winning

// TODO(cyx): determine if there's a better way to filter this out.
const deprecatedAppName = "All Applications"

func appTypeFor(v *string) string {
func typeFor(v *string) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Given this is in the same display package scope typeFor might not make sense. If we do plan to split up packages later, it would make sense then. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the idea is to split it later into packages. But will change it back for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed in 47b5645

Copy link
Contributor

@cyx cyx left a comment

Choose a reason for hiding this comment

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

Just a Q re: typeFor otherwise thanks for doing this 👍 💪

@Widcket Widcket merged commit d0d5dd2 into main Feb 26, 2021
@Widcket Widcket deleted the refactor/apps branch February 26, 2021 19:07
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.

2 participants