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.

Sign up for GitHub

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

Jump to bottom

Propagate the error message when a credential helper fails. #16012

Closed
wants to merge 1 commit into from
Closed

Propagate the error message when a credential helper fails. #16012

wants to merge 1 commit into from

Conversation

tjgq
Copy link
Contributor

@tjgq tjgq commented Aug 1, 2022

No description provided.

@tjgq tjgq requested a review from coeuvre August 1, 2022 14:59
@tjgq tjgq requested a review from a team as a code owner August 1, 2022 14:59
@tjgq
Copy link
Contributor Author

tjgq commented Aug 1, 2022

@Yannic

@sgowroji sgowroji added team-Remote-Exec Issues and PRs for the Execution (Remote) team awaiting-review PR is awaiting review from an assigned reviewer labels Aug 1, 2022
coeuvre
coeuvre reviewed Aug 1, 2022
View reviewed changes
...ava/com/google/devtools/build/lib/authandtls/credentialhelper/CredentialHelperException.java Outdated
package com.google.devtools.build.lib.authandtls.credentialhelper;

/** An {@link Exception} thrown while invoking a credential helper. */
public class CredentialHelperException extends RuntimeException {
Copy link
Member

@coeuvre coeuvre Aug 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Can this extend IOException?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I've also fixed the test assertions.

Yannic
Yannic approved these changes Aug 1, 2022
View reviewed changes
Copy link
Contributor

@Yannic Yannic left a comment

Choose a reason for hiding this comment

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

Thanks! Do we need to (a) cherry-pick to 5.3 and (b) also change the error for BES upload?

@tjgq
Copy link
Contributor Author

tjgq commented Aug 1, 2022

Thanks! Do we need to (a) cherry-pick to 5.3 and (b) also change the error for BES upload?

BES already includes the cause in the error message: https://cs.opensource.google/bazel/bazel/+/master:src/main/java/com/google/devtools/build/lib/buildeventservice/client/BuildEventServiceGrpcClient.java;l=183;drc=f141e149ac3210e417b31aec21afed267cdbb092

I'm not sure whether we're still accepting cherry-picks for 5.3, but I'll ask.

@tjgq
Copy link
Contributor Author

tjgq commented Aug 2, 2022

@bazel-io fork 5.3.0

@copybara-service copybara-service bot closed this in 24c16fc Aug 2, 2022
@sgowroji
Copy link
Member

sgowroji commented Aug 2, 2022

@bazel-io fork 5.3.0

@bazel-io bazel-io mentioned this pull request Aug 2, 2022
Closed
ShreeM01 pushed a commit that referenced this pull request Aug 2, 2022
@tjgq @ShreeM01
Propagate the error message when a credential helper fails.
b82db4d 
Closes #16012.

PiperOrigin-RevId: 464732834
Change-Id: If51ce914098ff17ffe23fa4614947e7f4a5088dc
@ShreeM01 ShreeM01 mentioned this pull request Aug 2, 2022
Merged
@ShreeM01
Copy link
Contributor

ShreeM01 commented Aug 2, 2022

Hey @tjgq we are trying to [cherry-pick] (#16030) the changes to release 5.3.0. But pre-submits checks are failing due to missing dependencies. So could you please help us in cherry-picking this changes to release 5.3.0? Thanks!

ShreeM01 added a commit that referenced this pull request Aug 3, 2022
@ShreeM01 @tjgq
Propagate the error message when a credential helper fails. (#16030)
6102d33 
Closes #16012.

PiperOrigin-RevId: 464732834
Change-Id: If51ce914098ff17ffe23fa4614947e7f4a5088dc

Co-authored-by: Tiago Quelhas <tjgq@google.com>
ShreeM01 added a commit that referenced this pull request Aug 9, 2022
@ShreeM01 @tjgq
@ted-xie @sgowroji @coeuvre
Release 5.3.0 (#16074)
2077c54 
* Updated Codeowners file (#16032)

* Propagate the error message when a credential helper fails. (#16030)

Closes #16012.

PiperOrigin-RevId: 464732834
Change-Id: If51ce914098ff17ffe23fa4614947e7f4a5088dc

Co-authored-by: Tiago Quelhas <tjgq@google.com>

* Migrate legacy desugar wrapper to new rlocation() (#16025)

RELNOTES: Fix for desugaring failure on Bazel+Android+Windows build scenario.
PiperOrigin-RevId: 464654335
Change-Id: I845093b134dc68ae541603008fe7fee701c7451c

Co-authored-by: Gowroji Sunil <48122892+sgowroji@users.noreply.github.com>

* Correctly report errors thrown by CommandLinePathFactory#create. (#16036)

This method is called while initializing the remote module [1]. Any exceptions
not derived from java.io.IOException cause a silent server crash.

[1] https://cs.opensource.google/bazel/bazel/+/master:src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java;l=436;drc=f3211f00ae08746b5794ab01d404c32b43146aba

Closes #16022.

PiperOrigin-RevId: 464997904
Change-Id: I87fd5eaeb562d849bb830d68f1bfbbf06f6e0ea9

Co-authored-by: Tiago Quelhas <tjgq@google.com>

* Fix an issue that `incompatible_remote_build_event_upload_respect_no_… (#16045)

* Fix an issue that `incompatible_remote_build_event_upload_respect_no_…

…cache` doesn't work with BwtB.

The root cause is we use `Path` as key to check which files are omitted, but it also compares the underlying filesystem. When BwtB is enabled, the filesystem for the file is different so we failed to mark the file as omitted.

This change fixes that by using `PathFragment` as key.

Fixes #15527.

Closes #16023.

PiperOrigin-RevId: 465284879
Change-Id: I49bbd7a6055e0f234b4ac86a224886bd85797f71

* Update ByteStreamBuildEventArtifactUploader changes

Removing the import as it is throwing the error

Co-authored-by: Chi Wang <chiwang@google.com>

Co-authored-by: Tiago Quelhas <tjgq@google.com>
Co-authored-by: Ted <tedx@google.com>
Co-authored-by: Gowroji Sunil <48122892+sgowroji@users.noreply.github.com>
Co-authored-by: Chi Wang <chiwang@google.com>
@ShreeM01 ShreeM01 removed the awaiting-review PR is awaiting review from an assigned reviewer label Sep 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@coeuvre coeuvre coeuvre approved these changes

@Yannic Yannic Yannic approved these changes

Assignees
No one assigned
Labels
team-Remote-Exec Issues and PRs for the Execution (Remote) team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@tjgq @sgowroji @ShreeM01 @coeuvre @Yannic