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

[6.0.0] [remote] Respect whether the server supports action cache updates #16724

Conversation

Yannic
Copy link
Contributor

@Yannic Yannic commented Nov 9, 2022

Only a subset of users may be allowed to update the action cache (e.g., only CI but not devs).

Today, there are 2 ways to achive the desired behavior:

  • GetCapabilities returning that all users are allowed to update, and UpdateActionResult returning an error that Bazel prints and ignores, or
  • have the users that are not allowed to update the action cache set --remote_upload_local_results=false.

Why don't we instead respect whether the remote cache supports uploading action results?

Note that this requires support from the remote system to fully work (i.e., it needs to return update_enabled = false for users that don't have permission). Otherwise, Bazel's behavior will be the same as before this change: failed UpdateActionResult do not cause the build to fail. The only change this introduces is that Bazel will no longer error if --remote_upload_local_results=true and GetCapabilities returning update_enabled = false.

Closes #16624.

PiperOrigin-RevId: 486901751
Change-Id: I0991f6891e21711df1e23ae0998a8bc95e2389bc

…ates

Only a subset of users may be allowed to update the action cache (e.g., only CI but not devs).

Today, there are 2 ways to achive the desired behavior:
  - `GetCapabilities` returning that all users are allowed to update, and `UpdateActionResult` returning an error that Bazel prints and ignores, or
  - have the users that are not allowed to update the action cache set `--remote_upload_local_results=false`.

Why don't we instead respect whether the remote cache supports uploading action results?

Note that this requires support from the remote system to fully work (i.e., it needs to return `update_enabled = false` for users that don't have permission). Otherwise, Bazel's behavior will be the same as before this change: failed `UpdateActionResult` do not cause the build to fail. The only change this introduces is that Bazel will no longer error if `--remote_upload_local_results=true` and `GetCapabilities` returning `update_enabled = false`.

Closes bazelbuild#16624.

PiperOrigin-RevId: 486901751
Change-Id: I0991f6891e21711df1e23ae0998a8bc95e2389bc
@Yannic Yannic requested a review from ShreeM01 as a code owner November 9, 2022 19:32
@Yannic
Copy link
Contributor Author

Yannic commented Nov 9, 2022

@meteorcloudy @kshyanashree PTAL

@ShreeM01 ShreeM01 added team-Remote-Exec Issues and PRs for the Execution (Remote) team awaiting-review PR is awaiting review from an assigned reviewer labels Nov 10, 2022
@ShreeM01 ShreeM01 requested a review from coeuvre November 10, 2022 00:27
@meteorcloudy meteorcloudy enabled auto-merge (squash) November 10, 2022 09:15
@meteorcloudy
Copy link
Member

@coeuvre Please LGTM if this looks good to be cherry-picked for 6.0 ;)

Copy link
Member

@coeuvre coeuvre left a comment

Choose a reason for hiding this comment

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

Yes, it's safe to cherry-pick this into 6.0.

@meteorcloudy
Copy link
Member

@Yannic Can you rebase?

@Yannic
Copy link
Contributor Author

Yannic commented Nov 10, 2022

thanks! done

@meteorcloudy meteorcloudy merged commit f589512 into bazelbuild:release-6.0.0 Nov 10, 2022
@Yannic Yannic deleted the yannic-cp-128d833fee99f8a43bc4de82cbec752e4ce6fb47 branch November 10, 2022 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review PR is awaiting review from an assigned reviewer team-Remote-Exec Issues and PRs for the Execution (Remote) team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants