Skip to content

Add MergeMode for toggling highlighting of merge conflicts#218

Open
LucaBernstein wants to merge 6 commits intogardener:mainfrom
LucaBernstein:modifications-merge-conflict-handling
Open

Add MergeMode for toggling highlighting of merge conflicts#218
LucaBernstein wants to merge 6 commits intogardener:mainfrom
LucaBernstein:modifications-merge-conflict-handling

Conversation

@LucaBernstein
Copy link
Copy Markdown
Member

How to categorize this PR?

/area usability
/kind enhancement

What this PR does / why we need it:
Currently, once a components.yaml file is present, its versions will not be updated anymore by resolve plain. This PR makes the glk update non-overridden versions.

Also, the "default-value"-handling that until now was only applied to the components.yaml, has been extended to all manifests by integrating it into WriteObjectsToFilesystem / ThreeWayMergeManifest.

Which issue(s) this PR fixes:
Fixes #210
Follow-up to #216

Special notes for your reviewer:
/cc @timuthy

Release note:

Add `MergeMode` for toggling highlighting of merge conflicts.

Introduces `MergeMode` (Silent/Informative) and threads it through
`threeWayMerge`, `threeWayMergeSection`, and `threeWayMergeSequence`.

In `MergeModeInformative`, when an operator-overridden scalar value
differs from the current GLK default, a `# glk default: <value>
# <-- glk-managed` line comment is appended to the value node.
For complex nodes (mappings/sequences), a head comment on the key
node signals the divergence.

`WriteComponentVectorFile`, `stripDefaultVersionComments`, and
`defaultVersionCommentMarker` are deleted. Their functionality is now
covered generically by `WriteObjectsToFilesystem(MergeModeInformative)``,
which plain.go already calls directly. The corresponding test describe
block is removed along with the now-unused imports.

Operator-comments are retained.
…l-sites

Existing callers pass `MergeModeSilent` (zero value, no behaviour change).
plain.go passes `MergeModeInformative` so the components.yaml file gets
GLK default annotations for operator-overridden component versions.
@gardener-prow gardener-prow bot requested a review from timuthy April 1, 2026 14:34
@gardener-prow gardener-prow bot added area/usability Usability related kind/enhancement Enhancement, improvement, extension cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 1, 2026
@timuthy
Copy link
Copy Markdown
Member

timuthy commented Apr 2, 2026

/assign

Copy link
Copy Markdown
Member

@timuthy timuthy left a comment

Choose a reason for hiding this comment

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

Thanks for taking care about of merge mode configuration!

Comment on lines +143 to +150
// GetMergeMode returns the configured MergeMode, defaulting to MergeModeInformative.
// It is safe to call on a nil receiver.
func (c *LandscapeKitConfiguration) GetMergeMode() MergeMode {
if c != nil && c.MergeMode != nil {
return *c.MergeMode
}
return MergeModeInformative
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Use a default value assignment instead?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Currently we are not defaulting for any option - or wdym?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I meant in Gardener or Kubernetes APIs in general. It should belong to https://github.com/gardener/gardener-landscape-kit/blob/main/pkg/apis/config/v1alpha1/defaults.go.

})
})

Describe("#WriteObjectsToFilesystem - MergeModeInformative", func() {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you make use of Context to run the different modes?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

updated the tests, ptal

resultValue = currentValue
if oldExists {
mergeNodeComments(oldValue, newValueNode, resultValue)
if mode == configv1alpha1.MergeModeInformative && !nodesEqual(newValueNode, currentValue, false) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
if mode == configv1alpha1.MergeModeInformative && !nodesEqual(newValueNode, currentValue, false) {
if mode == configv1alpha1.MergeModeInformative && !nodesEqual(oldValue, newValueNode, false) && !nodesEqual(newValueNode, currentValue, false) {

Otherwise the annotation is added every time for customized values.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

reworked the merge logic w.r.t. the default comments. ptal.


// GLKDefaultPrefix is the comment prefix for GLK-managed default annotations.
// It is exported so callers can use it as the strip anchor when removing annotations.
GLKDefaultPrefix = "# glk default: "
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
GLKDefaultPrefix = "# glk default: "
GLKDefaultPrefix = "# Attention - new default: "

Something like this?

resultValue.LineComment = annotation
}
default:
resultKeyNode.HeadComment = glkManagedHeadComment(resultKeyNode.HeadComment)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The comment is not added for lists. Instead, when a default values of a list item is changed, the item is completely appended to the list.

Example:

t0: Default

          - apiGroups: [ "core.gardener.cloud" ]
            apiVersions: [ "*" ]
            resources: [ "shoots" ]
            size: 750Ki

t1: Overwrite

          - apiGroups: [ "core.gardener.cloud" ]
            apiVersions: [ "*" ]
            resources: [ "shoots" ]
            size: 850Ki

t2: Default

          - apiGroups: [ "core.gardener.cloud" ]
            apiVersions: [ "*" ]
            resources: [ "shoots" ]
            size: 950Ki

t3: Result

          - apiGroups: [ "core.gardener.cloud" ]
            apiVersions: [ "*" ]
            resources: [ "shoots" ]
            size: 850Ki
          - apiGroups: [ "core.gardener.cloud" ]
            apiVersions: [ "*" ]
            resources: [ "shoots" ]
            size: 950Ki

@gardener-prow
Copy link
Copy Markdown

gardener-prow bot commented Apr 10, 2026

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from timuthy. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@gardener-prow gardener-prow bot added cla: no Indicates the PR's author has not signed the cla-assistant.io CLA. cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. cla: no Indicates the PR's author has not signed the cla-assistant.io CLA. labels Apr 10, 2026
@gardener-prow
Copy link
Copy Markdown

gardener-prow bot commented Apr 13, 2026

PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/usability Usability related cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. kind/enhancement Enhancement, improvement, extension needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Merge conflict handling

2 participants