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-140: integration tests on ci #263

Merged
merged 34 commits into from
May 7, 2021
Merged

CLI-140: integration tests on ci #263

merged 34 commits into from
May 7, 2021

Conversation

as-herzog
Copy link
Contributor

@as-herzog as-herzog commented Apr 23, 2021

Description

Runs integration tests in CI but skips them for forked PRs:
Screen Shot 2021-05-07 at 2 31 41 PM

Forked ex:

Screen Shot 2021-05-07 at 2 28 55 PM

References

https://auth0team.atlassian.net/browse/CLI-140

@as-herzog as-herzog changed the title chore: initial try at workflow change CLI-140: integration tests on ci Apr 23, 2021
AUTH0_CLI_CLIENT_DOMAIN: ${{ secrets.AUTH0_CLI_CLIENT_DOMAIN }}
AUTH0_CLI_CLIENT_ID: ${{ secrets.AUTH0_CLI_CLIENT_ID }}
AUTH0_CLI_CLIENT_SECRET: ${{ secrets.AUTH0_CLI_CLIENT_SECRET }}
AUTH0_CLI_REUSE_CONFIG: ${{ secrets.AUTH0_CLI_REUSE_CONFIG }}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if we need AUTH0_CLI_REUSE_CONFIG on the secrets. It should be always false on CI if understood the code correctly; there's no config file to reuse here.

Copy link
Contributor

Choose a reason for hiding this comment

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

what if we tweak the CLI codebase to look for all those ENV vars and skip the config file completely if the vars are present?

Copy link
Contributor

Choose a reason for hiding this comment

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

nvm, we don't want to support client id/secret auth mechanism on the code base. In any case, it could look for an ENV var with the access token and skip config file, but that has nothing to do with this PR.

@@ -38,11 +38,7 @@ lint:

# Build for the native platform
build:
go build -ldflags "$(CTIMEVAR)" -o auth0 cmd/auth0/main.go
Copy link
Contributor

Choose a reason for hiding this comment

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

good catch. bad resolved merge conflict I guess 🤦🏼

Domain: p.clientDomain,
AccessToken: token.AccessToken,
ExpiresAt: token.Expiry,
Scopes: requiredScopes,
Copy link
Contributor

@jfatta jfatta May 7, 2021

Choose a reason for hiding this comment

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

the CLI is triggering a login flow if the scopes on the tenant are not equal to the defined on the auth package, this is a way we have to "revalidate" tokens if we change the scopes used.

Evidently, it would be hard to maintain (keep in sync) the config generator regarding tenant and other login details.

I would like to explore that alternative of skipping all file-related config if an access token is defined. That way the generator would be repurposed on a command to login and write the access token to a var, no need to know the tenant entity at all.

Makefile Outdated
.PHONY: run-integration

# Delete all test apps created during integration testing
integration-cleanup:
./integration/test-cleanup.sh
.PHONY: integration-cleanup

integration: $(GOBIN)/auth0-cli-config-generator $(GOBIN)/commander run-integration integration-cleanup
integration: build $(GOBIN)/auth0-cli-config-generator $(GOBIN)/commander run-integration integration-cleanup
Copy link
Contributor

Choose a reason for hiding this comment

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

@as-herzog I kept the integration-cleanup when I solved merge conflicts. But I think that is swallowing potential error reports from commander when tests failed.

@as-herzog as-herzog marked this pull request as ready for review May 7, 2021 20:12
Copy link
Contributor

@Widcket Widcket left a comment

Choose a reason for hiding this comment

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

Awesome, thank you for doing this!

@Widcket Widcket merged commit e8cebd5 into main May 7, 2021
@Widcket Widcket deleted the cli-140-integration-on-ci branch May 7, 2021 22:04
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