-
Notifications
You must be signed in to change notification settings - Fork 374
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
ci: support external read-only cache for untrusted PRs #14570
Conversation
f51cf88
to
60534fa
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14570 +/- ##
=======================================
Coverage 93.59% 93.59%
=======================================
Files 2316 2316
Lines 206915 206915
=======================================
+ Hits 193654 193661 +7
+ Misses 13261 13254 -7 ☔ View full report in Codecov by Sentry. |
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.
You may want somebody more invested in the review. I approve of the intent, and I think it will work. I am not paying a lot of attention to how maintainable this will be.
io::run bazelisk "${args[@]}" test "${test_args[@]}" "${integration_test_args[@]}" \ | ||
//google/cloud/storage/tests/... | ||
} | ||
if [[ "${EXECUTE_INTEGRATION_TESTS}" == "true" ]]; then |
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.
Consider using getopt
and something like --skip-integration-tests=true
. Environment variables are global variables in disguise, one should use them sparingly
--test_env=GRPC_DEFAULT_SSL_ROOTS_FILE_PATH="${HOME}/roots.pem" \ | ||
//google/cloud/storage/tests/... | ||
} | ||
if [[ "${EXECUTE_INTEGRATION_TESTS}" == "true" ]]; then |
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.
Ditto, consider using an argument to the script.
@@ -45,7 +45,11 @@ function bazel::test_args() { | |||
) | |||
if [[ -n "${BAZEL_REMOTE_CACHE:-}" ]]; then | |||
args+=("--remote_cache=${BAZEL_REMOTE_CACHE}") | |||
args+=("--google_default_credentials") | |||
if [[ "${BAZEL_REMOTE_CACHE_RW_MODE}" == "READ_WRITE" ]]; then |
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.
Also consider something other than enviroment variables for these.
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.
Reviewable status: 0 of 9 files reviewed, 3 unresolved discussions (waiting on @coryan)
ci/gha/builds/macos-bazel.sh
line 53 at r1 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
Consider using
getopt
and something like--skip-integration-tests=true
. Environment variables are global variables in disguise, one should use them sparingly
created #14572
ci/gha/builds/windows-bazel.sh
line 49 at r1 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
Ditto, consider using an argument to the script.
Ack
ci/gha/builds/lib/bazel.sh
line 48 at r1 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
Also consider something other than enviroment variables for these.
Ack
While sccache is unavailable in a read_only mode, both the bazel-cache and vcpkg-cache can be used without credentials.
Integration tests, since they require credentials, will not be run by workflows started by test-runner-untrusted.yml.
This change is