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

fix: Correctly handle repo name when apply fine grained filtering #261

Merged

Conversation

honnix
Copy link
Contributor

@honnix honnix commented Jan 23, 2025

When using --useCquery or setting --bazelCommandOptions=--consistent_labels (while not using --useCquery), canonical repo name must be used for --fineGrainedHashExternalRepos, but with a leading @. In that case, we cannot remove all @ from source target name, otherwise nothing will never match --fineGrainedHashExternalRepos.

This PR fixes that by asking users to always provide apparent or canonical repo name (with @ or @@).

After this fix, it is not strictly necessary to use canonical name when --useCquery because we compare repo name without any @, but to keep it easier for users to follow, the readme says otherwise.

Note that this is a breaking change.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@honnix
Copy link
Contributor Author

honnix commented Jan 23, 2025

@tinder-maxwellelliott I'm thinking maybe it's better not to do this dance, but instead asking users to provide either apparent or canonical repo names with all the necessary @s. That would be a breaking change for sure, but I think that would make things a bit easier to handle and understand. What do you think?

@@ -1,9 +1,9 @@
package com.bazel_diff.bazel

import com.bazel_diff.log.Logger
import java.util.Calendar
Copy link
Contributor Author

Choose a reason for hiding this comment

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

bazel run format did this.

@honnix honnix force-pushed the fix-fine-grained-repo-filter branch from 4a41de5 to 3979ee4 Compare January 23, 2025 17:49
@honnix honnix marked this pull request as ready for review January 24, 2025 08:36
@tinder-maxwellelliott
Copy link
Collaborator

@tinder-maxwellelliott I'm thinking maybe it's better not to do this dance, but instead asking users to provide either apparent or canonical repo names with all the necessary @s. That would be a breaking change for sure, but I think that would make things a bit easier to handle and understand. What do you think?

I think that sounds good to me, I am wary of any all-targets querying in bzlmod anyhow

@honnix
Copy link
Contributor Author

honnix commented Jan 24, 2025

@tinder-maxwellelliott I'm thinking maybe it's better not to do this dance, but instead asking users to provide either apparent or canonical repo names with all the necessary @s. That would be a breaking change for sure, but I think that would make things a bit easier to handle and understand. What do you think?

I think that sounds good to me, I am wary of any all-targets querying in bzlmod anyhow

Cool. I will rework this PR a bit then.

@honnix honnix force-pushed the fix-fine-grained-repo-filter branch from 3979ee4 to f8ddde0 Compare January 25, 2025 13:19
@honnix honnix changed the title fix: Respect leading @ when apply fine grained filtering fix: Correctly handle repo name when apply fine grained filtering Jan 25, 2025
@tinder-maxwellelliott tinder-maxwellelliott merged commit 7c8e9b8 into Tinder:master Jan 27, 2025
6 of 8 checks passed
@honnix honnix deleted the fix-fine-grained-repo-filter branch January 27, 2025 17:52
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.

3 participants