[credentialhelper] Implement invoking credential helper as subprocess #15861
Conversation
|
@tjgq PTAL |
| GSON.toJson(GetCredentialsRequest.newBuilder().setUri(uri).build(), stdin); | ||
| } | ||
|
|
||
| process.waitFor(); |
There was a problem hiding this comment.
Should we have a timeout here to avoid hanging Bazel indefinitely? (Just some food for thought, this can be done in a followup)
There was a problem hiding this comment.
FYI, I will change the timeout error message before submitting internally, because Duration#toString doesn't produce a human-readable format.
| throw new IOException( | ||
| String.format( | ||
| Locale.US, | ||
| "Failed to get credentials from helper (exited with code %d): %s", |
There was a problem hiding this comment.
Should we be a little more descriptive here? e.g. Failed to get credentials for url {url} from helper {path} ...
Same for the other two IOExceptions below.
| } | ||
|
|
||
| private static boolean isWindows() { | ||
| return File.separatorChar == '\\'; |
There was a problem hiding this comment.
It looks like we've already got a util for this elsewhere: https://cs.opensource.google/bazel/bazel/+/master:src/main/java/com/google/devtools/build/lib/bazel/rules/BazelStrategyModule.java;l=75;drc=c3143bbed207fb316866f938676878eae31f121a
| .put("header1", ImmutableList.of("value1")) | ||
| .build()); | ||
| } | ||
| } |
There was a problem hiding this comment.
IMO there's a couple more things we should test:
- helper returns empty object ("no headers required for this URL")
- environment variables (i.e., pass an env var to the helper and embed it in the response to verify that it was passed in correctly)
There was a problem hiding this comment.
Added a test for env vars and the CWD of the helper. I don't think we need to test parsing many responses since we have many tests for parsing GetCredentialsResponse from JSON
| eprint("Usage: test_credential_helper <command>") | ||
| return 1 | ||
|
|
||
| if argv[1] == "get": |
There was a problem hiding this comment.
Prefer to exit early on error conditions to keep the indentation level as small as possible:
if argv[1] != "get":
eprint("Unknown command ...")
return 1
...
sgowroji
added
team-Remote-Exec
|
PTAL |
|
@bazel-io flag |
bazel-io
added
the
potential release blocker
|
@bazel-io fork 5.3.0 |
bazel-io
removed
the
potential release blocker
Progress on https://github.com/bazelbuild/proposals/blob/main/designs/2022-06-07-bazel-credential-helpers.md Progress on bazelbuild#15856 Closes bazelbuild#15861. PiperOrigin-RevId: 461159351 Change-Id: I28eb4817ced8db8f095a1f35092fdefba28e0ede
…#15900) Progress on https://github.com/bazelbuild/proposals/blob/main/designs/2022-06-07-bazel-credential-helpers.md Progress on #15856 Closes #15861. PiperOrigin-RevId: 461159351 Change-Id: I28eb4817ced8db8f095a1f35092fdefba28e0ede Co-authored-by: Yannic Bonenberger <yannic@engflow.com>
Progress on https://github.com/bazelbuild/proposals/blob/main/designs/2022-06-07-bazel-credential-helpers.md Progress on bazelbuild#15856 Closes bazelbuild#15861. PiperOrigin-RevId: 461159351 Change-Id: I28eb4817ced8db8f095a1f35092fdefba28e0ede
Progress on https://github.com/bazelbuild/proposals/blob/main/designs/2022-06-07-bazel-credential-helpers.md Progress on bazelbuild#15856 Closes bazelbuild#15861. PiperOrigin-RevId: 461159351 Change-Id: I28eb4817ced8db8f095a1f35092fdefba28e0ede
ShreeM01
removed
the
awaiting-review
Progress on https://github.com/bazelbuild/proposals/blob/main/designs/2022-06-07-bazel-credential-helpers.md
Progress on #15856