feat: add PatchArgs API type to populate patch options

[adoramshoval]

This commit converts the Options section of a patch into an object instead of map. This allows better clarification of the available options and should prevents human typing errors. It is the base branch for an already existing branch adding the option allowNoTargetMatch, see #5917 .

This was requested as part of #4715 , see https://github.com/kubernetes-sigs/kustomize/pull/4715/files#r1278209456.

[k8s-ci-robot]

Hi @adoramshoval. 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-sigs/prow repository.

[sarab97]

/ok-to-test

[sarab97]

This looks good! The nil handling for Options is well implemented, excellent work. However, I'd suggest alternative of ensuring the Options struct always exists instead of allowing nil pointers. Unlike Target, this replaces an existing map[string]bool workflow, so checking for struct nil before accessing members might break existing workflows that expect options to always be available and add unnecessary validation overhead at each access point

Since this changes a map structure, it would be safer to guarantee the Options struct always exists (even if empty) to ensure existing workflows aren't affected.

[adoramshoval]

This looks good! The nil handling for Options is well implemented, excellent work. However, I'd suggest alternative of ensuring the Options struct always exists instead of allowing nil pointers. Unlike Target, this replaces an existing map[string]bool workflow, so checking for struct nil before accessing members might break existing workflows that expect options to always be available and add unnecessary validation overhead at each access point

Since this changes a map structure, it would be safer to guarantee the Options struct always exists (even if empty) to ensure existing workflows aren't affected.

Hi @sarab97 , thanks for your review! Before I jump in, let me make sure I got you right. Instead of passing Options as a pointer, you would like me to pass its value so it would get intialized with default values, correct? I think that makes a lot of sense, thanks a lot 🤗 .

[adoramshoval]

Hi, I have stummbled upon an issue while trying to reimplement the Patch struct now using the value of PatchArgs and I would like your help. Since a struct does not have a zero value, having the value of PatchArgs adds an empty Options: {} field to a processed kustomization.yaml file going through the localizer which causes some tests to fail, for example TestTargetNestedInScope.

The test expects:

apiVersion: kustomize.config.k8s.io/v1beta1
        kind: Kustomization
        patches:
        - patch: |-
            - op: replace
              path: /some/existing/path
              value: new value
          target:
            kind: Deployment
            labelSelector: env=dev

but the actual output is:

apiVersion: kustomize.config.k8s.io/v1beta1
        kind: Kustomization
        patches:
        - options: {}
          patch: |-
            - op: replace
              path: /some/existing/path
              value: new value
          target:
            kind: Deployment
            labelSelector: env=dev

Notice the added Options: {}.

@sarab97 , @koba1t , @stormqueen1990 I would like your assistance on how to proceed from here since I am not sure which direction would be better. Also, let me know if this change isn't necessary as a whole.

[adoramshoval]

Any thoughts? @sarab97 , @koba1t

[stormqueen1990]

@sarab97 , @koba1t , @stormqueen1990 I would like your assistance on how to proceed from here since I am not sure which direction would be better. Also, let me know if this change isn't necessary as a whole.

Hi there, @adoramshoval!

I did some testing with a simpler piece of code and noticed the same behaviour. Your assessment makes sense to me: a struct cannot have a zero value and therefore will be serialized into YAML/JSON even when not explicitly specified.

@sarab97 would you agree to go back to the original implementation with a pointer so we can keep the serialization behaviour of kustomize?

[sarab97]

@sarab97 would you agree to go back to the original implementation with a pointer so we can keep the serialization behaviour of kustomize?

If its fine by you yeah sure, even though it adding option as empty it is not expected. So Im guessing the original approach is the only way.

[adoramshoval]

Then I'll proceed with this. Thanks guys!

[adoramshoval]

/retest

[adoramshoval]

I made the changes, I hope this won't fail lint tests now. Notice that I also modified plugin/builtin/patchtransformer/PatchTransformer.go, I fingured it makes more sense for it to also include the the change I am introduing here. Let me know what do you think.

[koba1t]

/lgtm /approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adoramshoval, koba1t

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

The pull request process is described here

Needs approval from an approver in each of these files:

• ~~OWNERS~~ [koba1t]

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