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 tenants open and apis open [CLI-101] [CLI-133] #253

Merged
merged 8 commits into from
Apr 19, 2021
Merged

Conversation

Widcket
Copy link
Contributor

@Widcket Widcket commented Apr 18, 2021

Description

This PR adds the following commands:

  • tenants open that opens the tenant's settings page on the Dashboard. It takes the tenant's domain as a parameter.
  • apis open that opens the API's settings page on the Dashboard. It takes either the API Id or audience as a parameter.

This is important as it provides a handy escape hatch, because the CLI does not support every parameter available for APIs, and does not allow to configure tenants ATM.

This PR also fixes a bug that caused the open logic to not work with PUS1 tenants, that do not have the region in the URL.

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

@Widcket Widcket requested a review from a team April 18, 2021 22:17
Copy link
Contributor

@jfatta jfatta left a comment

Choose a reason for hiding this comment

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

looks good. minor suggestions inline.
thanks

}

// Heuristics to determine if this a valid ID, or an audience value
if _, err := url.ParseRequestURI(inputs.ID); err == nil || len(inputs.ID) != 24 {
Copy link
Contributor

Choose a reason for hiding this comment

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

what's 24coming from ? is inputs.ID a url? in that case I wonder if we can make it more explicit on the command help

Copy link
Contributor Author

Choose a reason for hiding this comment

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

24 is the string length of the ID. Audiences are usually URLs, but not necessarily, so there is that additional heuristic to help disambiguate. I'll make it more explicit on the command help and add a comment explaining the values, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 62c64d5

inputs.ID = auth0.StringValue(api.ID)
return nil
}); err != nil {
return fmt.Errorf("An unexpected error occurred while trying to get the API Id for '%s': %w", inputs.ID, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

NAB: I know we're doing this in other places already, but I'd suggest starting error messages with lowercase (as well as avoiding ending punctuation). More context here: https://github.com/golang/go/wiki/CodeReviewComments#error-strings

Copy link
Contributor Author

@Widcket Widcket Apr 19, 2021

Choose a reason for hiding this comment

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

Yes, the thing is that the reasoning behind that best practice is that errors are usually printed among other information, so there should not be a sentence-cased string in the middle of other text. In this particular case, that error message is going to be shown as-is, so we want the casing. We'll totally revisit this in the future, though, when we rethink the error handling.

@@ -669,7 +667,7 @@ func openAppCmd(cli *cli) *cobra.Command {
Args: cobra.MaximumNArgs(1),
Short: "Open application settings page in Auth0 Manage",
Long: "Open application settings page in Auth0 Manage.",
Example: `auth0 apps open <id>`,
Example: "auth0 apps open <id>",
Copy link
Contributor

Choose a reason for hiding this comment

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

from prev comment, If id could adopt different forms (I'm not sure) I'd suggest including some placeholders here in the example section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do, thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went with auth0 apps open <id|audience> in 62c64d5

func (c *cli) tenantPickerOptions() (pickerOptions, error) {
tens, err := c.listTenants()
if err != nil {
return nil, fmt.Errorf("Unable to load tenants due to an unexpected error: %w", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above.

internal/cli/tenants.go Outdated Show resolved Hide resolved
@Widcket Widcket merged commit 6c564a6 into main Apr 19, 2021
@Widcket Widcket deleted the open-commands branch April 19, 2021 22:41
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