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

Adapt ambient OIDC tests to support interactive flow for local testing #576

Merged
merged 10 commits into from
Mar 23, 2023

Conversation

tnytown
Copy link
Collaborator

@tnytown tnytown commented Mar 22, 2023

Summary

Introduces a new Makefile target test-oidc. This runs ambient OIDC tests with tokens from sigstore get-identity-token.

Partially fixes #570.

Signed-off-by: Andrew Pan <a@tny.town>
Signed-off-by: Andrew Pan <a@tny.town>
@tnytown tnytown added component:tests Unit and integration tests safe to test labels Mar 22, 2023
@tnytown tnytown marked this pull request as draft March 22, 2023 21:30
Signed-off-by: Andrew Pan <a@tny.town>
Signed-off-by: Andrew Pan <a@tny.town>
@tnytown tnytown marked this pull request as ready for review March 22, 2023 21:36
@tnytown tnytown requested a review from woodruffw March 22, 2023 21:37
Signed-off-by: Andrew Pan <a@tny.town>
Makefile Outdated
Comment on lines 86 to 90
.PHONY: test-oidc
test-oidc: TEST_ENV += \
SIGSTORE_IDENTITY_TOKEN_production=$$($(MAKE) -s run ARGS="get-identity-token") \
SIGSTORE_IDENTITY_TOKEN_staging=$$($(MAKE) -s run ARGS="--staging get-identity-token")
test-oidc: test
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Let's name this test-interactive, to emphasize that it'll require user interaction.

Comment on lines -128 to +134
print("Waiting for browser interaction...")
print("Waiting for browser interaction...", file=sys.stderr)
else:
server.enable_oob()
print(
f"Go to the following link in a browser:\n\n\t{server.auth_endpoint}"
f"Go to the following link in a browser:\n\n\t{server.auth_endpoint}",
file=sys.stderr,
Copy link
Member

Choose a reason for hiding this comment

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

Nice!

@woodruffw
Copy link
Member

Let's also document this new make target in the CONTRIBUTING.md, as another test entrypoint.

Signed-off-by: Andrew Pan <a@tny.town>
@tnytown tnytown requested a review from woodruffw March 22, 2023 22:24
Signed-off-by: Andrew Pan <a@tny.town>
@tnytown tnytown self-assigned this Mar 23, 2023
woodruffw
woodruffw previously approved these changes Mar 23, 2023
Copy link
Member

@woodruffw woodruffw left a comment

Choose a reason for hiding this comment

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

LGTM. get-identity-token should also work automatically with ambient credentials, so it might be good to add another test job in CI that makes sure we don't accidentally regress the make test-interactive target.

Signed-off-by: Andrew Pan <a@tny.town>
Signed-off-by: Andrew Pan <a@tny.town>
@woodruffw woodruffw merged commit 7904283 into sigstore:main Mar 23, 2023
@woodruffw woodruffw deleted the andrew/issue/570 branch March 23, 2023 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:tests Unit and integration tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reduce our dependency on ambient OIDC tests for error cases
2 participants