-
Notifications
You must be signed in to change notification settings - Fork 86
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 remote bazel integration test #6411
Conversation
c4102a0
to
f737538
Compare
82fddff
to
fecc1a8
Compare
999f0eb
to
b529339
Compare
8db10f5
to
b9dcc8c
Compare
b9dcc8c
to
71684c8
Compare
0d1ce3f
to
3451788
Compare
enterprise/server/test/integration/remote_bazel/remote_bazel_test.go
Outdated
Show resolved
Hide resolved
repoName := "private-test-repo" | ||
username := "maggie-lou" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you move this to the buildbuddy-io
org?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The repo is under the buildbuddy org, the token is under my account (you can't create a PAT for an org, but I could create a dummy account)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Decided that it will probably be more annoying to track a password for a service account. Anyone can re-generate this PAT if they need, even if I leave the company and we can't access this token or something
// This token grants read-only access to a private dummy repo. | ||
// Github will invalidate tokens that are committed, so format it so that Github | ||
// won't catch it. | ||
personalAccessToken := fmt.Sprintf("github_pat_%s", "11ADKOFLI06Ob3cnrygcQa_gwLj0orOD3EILEu2k9pJ9EUkmXW6uM0k47k0uPOWInCDPHNC444JmQ8oN62") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should avoid checking in access tokens, even though it only grants access to an empty repo - could you use the gh app token env var that is already passed to the workflow, so that this authenticates as the app installation rather than a person?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain more what you're describing? This is a /buildbuddy repo test, so are you describing the /buildbuddy app token?
Also as a somewhat related note - I was playing around with accessing BB secrets from within a go test, and the only way I could get it to work was if I added --test_env=SECRET_NAME
to the bazel command in buildbuddy.yaml. This passes the secret to all the tests, which may not be ideal (may be okay in this case, but could get messy if a lot of tests need to reference different secrets)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored to pull PAT from a secret (Also deleted the old PAT that was in our git history)
a1fa191
to
39c3138
Compare
… remote run completes
39c3138
to
946067e
Compare
946067e
to
faf6ffc
Compare
@@ -221,6 +221,7 @@ common --@io_bazel_rules_docker//transitions:enable=false | |||
# Firecracker tests can only be run on Linux machines with bare execution, and we want to avoid a hard dependency | |||
# on Docker for development | |||
test --test_tag_filters=-docker,-bare | |||
build --build_tag_filters=-secrets |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think this should be added to --test_tag_filters
above - it's fine to have the target built locally (to detect broken builds) but just not execute the test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to add this because the build was failing on the unauthenticated Github Actions build because we try to inject secrets
Related issues: N/A