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-27: basic integration tests. #122

Merged
merged 21 commits into from
Mar 21, 2021
Merged

Conversation

rene00
Copy link
Contributor

@rene00 rene00 commented Mar 4, 2021

Description

Initial attempt at basic integration tests.

References

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

@rene00 rene00 self-assigned this Mar 4, 2021
@rene00 rene00 force-pushed the cli-27-basic-integration-tests branch 2 times, most recently from 4207599 to 8da4efe Compare March 16, 2021 22:54
@rene00 rene00 force-pushed the cli-27-basic-integration-tests branch from 8da4efe to 9db1711 Compare March 16, 2021 22:57
@rene00 rene00 marked this pull request as ready for review March 16, 2021 22:59
@rene00 rene00 requested a review from a team March 16, 2021 22:59
@rene00
Copy link
Contributor Author

rene00 commented Mar 16, 2021

There are no github action changes included in this PR. I'm working through understanding how we can set this up safely.

@Widcket Widcket requested review from cyx and jfatta March 19, 2021 17:38
@@ -0,0 +1,206 @@
// auth0-cli-config-generator: A command that generates a valid config file that can be used with auth0-cli.
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for these small doc touches. Makes it feel 👌

Copy link
Contributor

Choose a reason for hiding this comment

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

General question on placement: should we put this in ./cmd/ instead? If not, curious to hear your thought process.

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 only real use is to generate a config for integration testing by a CI pipeline (or a noop if a valid config exists already) so storing it outside of where it could add cognitive overhead for contributors not working on tooling for integration tests was the idea. I also wanted to ensure it didnt depend on any code within ./internal/ to keep it siloed and independent. Happy to move it to ./cmd/ and though -- just let me know.

Comment on lines +59 to +69
type config struct {
DefaultTenant string `json:"default_tenant"`
Tenants map[string]tenant `json:"tenants"`
}

type tenant struct {
Name string `json:"name"`
Domain string `json:"domain"`
AccessToken string `json:"access_token,omitempty"`
ExpiresAt time.Time `json:"expires_at"`
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Note for the future: we can probably consider re-using some of the same entities we have in the CLI since they resemble each other.

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 made a comment on keeping it siloed but also happy to re-use entities in the CLI. I didn't want to add the coupling.

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 some general questions on placement, but not a real blocker. 👍

@rene00 rene00 merged commit b76ea32 into main Mar 21, 2021
@rene00 rene00 deleted the cli-27-basic-integration-tests branch March 21, 2021 22:51
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.

4 participants