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

[RB] Try to set default branch env var for better snapshot matching #6085

Merged
merged 2 commits into from
Mar 11, 2024

Conversation

maggie-lou
Copy link
Contributor

@maggie-lou maggie-lou commented Mar 6, 2024

The snapshot key is based off the git branch. If we do not have a snapshot for a branch, we support falling back to snapshots for default branches, as specified in the GIT_REPO_DEFAULT_BRANCH and GIT_BASE_BRANCH env vars.
This change will enable more snapshot hits.

Related issues: N/A

@maggie-lou
Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @maggie-lou and the rest of your teammates on Graphite Graphite

@maggie-lou maggie-lou force-pushed the rb_default_branch branch 2 times, most recently from 8ea4b07 to 7cb3f4b Compare March 6, 2024 18:09
@maggie-lou maggie-lou changed the title [RB] Try to set a default branch env var for better snapshot matching [RB] Try to set default branch env var for better snapshot matching Mar 6, 2024
@@ -197,6 +188,13 @@ func (r *runnerService) createAction(ctx context.Context, req *rnpb.RunRequest,
})
}

for k, v := range req.GetEnv() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When generating fallback snapshot keys, we only look at cmd.EnvironmentVariables, and not at the --env_overrides flag

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense - do we need env_overrides anymore after this change or can we remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No we don't. Cleaned up in workflow service too. Don't know why I didn't implement this way to begin with...

@maggie-lou maggie-lou marked this pull request as ready for review March 6, 2024 21:09
@@ -22,7 +22,7 @@ jobs:

- name: Test
run: |
bb remote test //... \
bb remote --env=GIT_REPO_DEFAULT_BRANCH=master test //... \
Copy link
Member

Choose a reason for hiding this comment

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

revert before merging?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The remote bazel code determines the default branch from the local git state. The github runner only checks out one commit, so we'd have to do that hacky thing where we briefly checkout master in order to take advantage of that. It's easier to just explicitly pass the flag

@@ -197,6 +188,13 @@ func (r *runnerService) createAction(ctx context.Context, req *rnpb.RunRequest,
})
}

for k, v := range req.GetEnv() {
Copy link
Member

Choose a reason for hiding this comment

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

Makes sense - do we need env_overrides anymore after this change or can we remove it?

@maggie-lou maggie-lou merged commit 2861eba into master Mar 11, 2024
21 checks passed
@maggie-lou maggie-lou deleted the rb_default_branch branch March 11, 2024 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants