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

[CLI-62] cli command to list tenants available #162

Merged
merged 10 commits into from
Mar 15, 2021
Merged

[CLI-62] cli command to list tenants available #162

merged 10 commits into from
Mar 15, 2021

Conversation

bright-poku
Copy link
Contributor

Description

add a command in internal/cli/tenants.go that lists in a table the tenants available to the user:

auth0 tenants list

+-------------------+
| AVAILABLE TENANTS |
+-------------------+
| bap-tenants       |
| bpoku             |
| test-bpoku        |
+-------------------+

References

https://auth0team.atlassian.net/browse/CLI-62?atlOrigin=eyJpIjoiMTlhZjY0ZTY5NTk3NDFkMjk2NjNlZjAyMDg4M2ZlNTEiLCJwIjoiaiJ9

Testing

Describe how this can be tested by reviewers. Be specific about anything not tested and reasons why. If this library has unit and/or integration testing, tests should be added for new functionality and existing tests should complete without errors.

Please include any manual steps for testing end-to-end or functionality not covered by unit/integration tests.

Also include details of the environment this PR was developed in (language/platform/browser version).

  • 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
  • [X ] The correct base branch is being used, if not master

@bright-poku bright-poku requested a review from Widcket March 15, 2021 13:56
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. I left some feedback inline.
additionally, it would be nice to add tenants_test.go at this point (but not a blocker for this PR)

return cmd
}

func listTenantCmd(cli *cli) *cobra.Command {
cmd := &cobra.Command{
Use: "list",
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest adding the ls command alias.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea, we could alias ls all the list commands.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do you want to create a seperate ticket for that? ls all the list commands. I will make the changes for this PR

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, I'll create a ticket for that

return fmt.Errorf("Unable to load tenants due to an unexpected error: %w", err)
}

tenNames := make([]string, len(listTenant))
Copy link
Contributor

@jfatta jfatta Mar 15, 2021

Choose a reason for hiding this comment

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

can we move the render logic (ln34 to ln46 here) to the display package? most (all should) follow this approach. Besides consistency, it is going to apply common logic to display 1 vs several results, json output, etc.

@Widcket Widcket requested a review from jfatta March 15, 2021 21:29
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.

🚀

@Widcket Widcket merged commit 87d664b into main Mar 15, 2021
@Widcket Widcket deleted the CLI-62 branch March 15, 2021 22:48
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.

3 participants