fix: add integer attributes to amend_artifact for explicit unset state #1499
+157
−3
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JonathanPerry651
requested review from
cheister,
jin and
shs96c
as code owners
JonathanPerry651
force-pushed
the
fix/maven-amend-artifact-int-attrs
branch
from
c3bdbeb to
24ebf47
Compare
Collaborator
|
Ooof. Good catch, and thank you for the PR. I've started a conversation on the Bazel Slack to see if there's a nice solution for this. Looking at the boolean attributes, I think we'd be safe if we only allowed the root module to set the boolean properties for both |
Contributor
Author
|
@shs96c - the slack convo seems somewhat inconclusive; what shall we do here? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a bit foul, but it's the only way I could see of actually passing through an explicit False to any of these attrs. I think I need this because junit is coming into my repo as testonly, and I want to amend that to False (I have some other deps that directly depend on junit).
If this is the wrong approach for whatever reason, I'm super-open to pushback!
Thank you