Keep comment when modify anything in kustomization.yaml

[wingyplus]

Use kyaml/yaml/comments.CopyComments to copy comments from the original kustomization source file to the updated file.

Fixes #5158

[k8s-ci-robot]

Hi @wingyplus. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

[wingyplus]

The PR still lacks the test but I want some feedback on it. 🙇

[varshaprasad96]

@wingyplus could you please rebase the changes onto master.

[k8s-ci-robot]

New changes are detected. LGTM label has been removed.

[wingyplus]

@varshaprasad96 Sorry for reply to you lately. The PR got rebased.

One thing I want to discuss is about behaviour change introduced by PR. The comment still keep but blank lines got consumed by CopyComments. Here is test that broken:

--- FAIL: TestPreserveCommentsWithAdjust (0.00s)
    kustomizationfile_test.go:278: Mismatch (-expected, +actual):
          bytes.Join({
        -       "\n\n\n\n",
                "# Some comments\n# This is some comment we should preserve\n# don'",
                "t delete it\n",
        -       "  ",
                "# See which field this comment goes into\nresources:\n- ../namespa",
                "ces\n- pod.yaml\n- service.yaml",
        -       "\n",
                "\napiVersion: kustomize.config.k8s.io/v1beta1\nkind: kustomization",
        -       "\n",
                "\n# something you may want to keep\nvars:\n- fieldref:\n    fieldPat",
                "h: metadata.name\n  name: MY_SERVICE_NAME\n  objref:\n    apiVersio",
                "n: v1\n    kind: Service\n    name: my-service",
        -       "\n",
                "\n# some descriptions for the patches",
        -       "\n",
                "\npatchesStrategicMerge:\n- service.yaml\n- pod.yaml\n# generator op",
                "tions\ngeneratorOptions:\n  disableNameSuffixHash: true\n",
          }, "")
--- FAIL: TestFixPatchesFieldForExtendedPatch (0.00s)
    kustomizationfile_test.go:321: Mismatch (-expected, +actual):
          bytes.Join({
        -       "\n",
                "patches:\n- path: patch1.yaml\n  target:\n    kind: Deployment\n- pa",
                "th: patch2.yaml\n  target:\n    kind: Service\napiVersion: kustomiz",
                "e.config.k8s.io/v1beta1\nkind: Kustomization\n",
          }, "")
--- FAIL: TestCommentsWithDocumentSeperatorAtBeginning (0.00s)
    kustomizationfile_test.go:368: Mismatch (-expected, +actual):
          bytes.Join({
        -       "\n\n\n",
                "# Some comments\n# This is some comment we should preserve\n# don'",
                "t delete it\napiVersion: kustomize.config.k8s.io/v1beta1\nkind: Ku",
                "stomization",
        -       "\n",
                "\nnamespace: mynamespace\n",
          }, "")
FAIL
FAIL    sigs.k8s.io/kustomize/kustomize/v5/commands/internal/kustfile   0.347s

If you (and reviewer) are ok with this. I can continue on this PR to fix the test.

[annasong20]

/ok-to-test

[annasong20]

Hi @wingyplus, thanks for the PR!

The current fix works: re-parsing the original kustomization into an *RNode and copying its comments. However, given that it ignores spacing and we're really only missing trailing comments after the last field (see https://github.com/kubernetes-sigs/kustomize/issues/5158#issuecomment-1604907095), a simpler solution would be to modify the existing code that parses comments, parseCommentedFields. For example, just thinking off the top of my head, you could

• add a lastComments field to the kustomizationFile struct set in parseCommentedFields
• make the field in commentedField optional and append a commentedField containing only a comment to originalFields

You can also add the test demonstrating the bug before you provide the fix. That way, you can check your implementation as you progress. Your reviewer can also more easily pinpoint the issue both at a glance and if they pull down your PR to explore.

Hope this helps!

[wingyplus]

Thanks @annasong20 for the guidance. Let me play around with it this week.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: varshaprasad96, wingyplus Once this PR has been reviewed and has the lgtm label, please assign annasong20 for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

• OWNERS

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

[wingyplus]

Hi, @annasong20, sorry for the late reply. The trailing comment at the end of the file for the simple case was solved. I'm curious on trailing comment after field or array element. Let's see the example:

What's a position should be when we have trailing comment after key?

Input:

apiVersion: ...
kind:...
resources: # Trailing comment

Output:

apiVersion: ...
kind:...
# Trailing comment
resources: 

Or

apiVersion: ...
kind:...
resources: # Trailing comment

And another case is trailing comment after array element.

apiVersion: ...
kind:...
resources: 
- deployment.yaml # Trailing comment

Output:

apiVersion: ...
kind:...
resources: 
- deployment.yaml # Trailing comment

Or

apiVersion: ...
kind:...
# Trailing comment
resources: 
- deployment.yaml

I think leaving trailing comments as it is good. Its benefit to Kpt to enable template value replacement.

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle stale
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle rotten
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Reopen this PR with /reopen
• Mark this PR as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

[k8s-ci-robot]

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Reopen this PR with /reopen
• Mark this PR as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

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/test-infra repository.