fix: overwrite attribute values when not mergeable #1952
Conversation
jbedard
force-pushed
the
set-unknown-attributes
branch
8 times, most recently
from
1a675a4 to
06fe94e
Compare
merger/merger.go
Outdated
| } | ||
| } | ||
|
|
||
| isManaged := func(r *rule.Rule, attr string) bool { | ||
| // Name and visibility are uniquely managed by gazelle. | ||
| if attr == "name" || attr == "visibility" { |
There was a problem hiding this comment.
I actually don't think visibility should be here but it causes too many changes in this repo atm. I think those changes are actually correct but they could be breaking such as reducing the visibility of //internal/....
jbedard
force-pushed
the
set-unknown-attributes
branch
from
06fe94e to
5a35779
Compare
fmeum
force-pushed
the
set-unknown-attributes
branch
from
5a35779 to
697c5bf
Compare
jbedard
force-pushed
the
set-unknown-attributes
branch
6 times, most recently
from
23e8768 to
40bd8f4
Compare
rule/merge.go
Outdated
| @@ -41,6 +41,9 @@ import ( | |||
| // marked with a "# keep" comment, values in the attribute not marked with | |||
| // a "# keep" comment will be dropped. If the attribute is empty afterward, | |||
| // it will be deleted. | |||
| // | |||
| // If src has an attribute not present in 'mergeable' and not marked with a | |||
There was a problem hiding this comment.
Should this be
| // If src has an attribute not present in 'mergeable' and not marked with a | |
| // If dst has an attribute not present in 'mergeable' and not marked with a |
There was a problem hiding this comment.
I think this comment is correct as-is, although maybe confusing.
This is pointing out that attributes in src will overwrite those in dst if they are unknown and the value in dst is not marked as # keep.
There was a problem hiding this comment.
I've updated the comment a bit. See the fixup commits.
jbedard
force-pushed
the
set-unknown-attributes
branch
from
40bd8f4 to
e710945
Compare
fmeum
force-pushed
the
set-unknown-attributes
branch
from
08ca365 to
02d0808
Compare
I'm seeing 1000s of build files behaving differently, need to dig a bit deeper. Please hold off on merging for now |
|
@tyler-french what types of changes are you seeing? Are effected attributes in the |
|
@tyler-french have you had a chance to look at this? I don't understand why you'd ever want |
|
@tyler-french any news on this? We've been waiting for this fix at Adobe. |
|
The main issues we are seeing are:
I will try to make reproductions with public code sometime soon |
|
Are those changes all in rules_go targets or in custom languages you have? |
jbedard
force-pushed
the
set-unknown-attributes
branch
from
02d0808 to
87ba5e7
Compare
|
@tyler-french any updates here? Even just ideas for tests I could write to reproduce the issues you're finding... |
Fix #1962
What type of PR is this?
What package or component does this PR mostly affect?
What does this PR do? Why is it needed?
Allow updating non-mergeable/resolveable attributes.
Which issues(s) does this PR fix?
Fixes #1962
Other notes for review