convert resources to ResourceGenerator

[koba1t]

#4402

[k8s-ci-robot]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[koba1t]

I have a problem with the import cycle not allowed error.
The accumurateTarget function is processing for kustomization.yaml, and I think ResourceGenerator needs exec this function due we need to process another kustomozation.yaml in the nested subdirectories.
But the target package contains the accumurateTarget function and imports the builtin package, which contains every generator code. That means if we want to call accumurateTarget in any generator plugins, that happens to cycle import for go codes and can't compile.

So Do you have any idea to fix this? I need your help resolving this code structure problem.

[k8s-ci-robot]

This PR has multiple commits, and the default merge method is: merge.
You can request commits to be squashed using the label: tide/merge-method-squash

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.

[natasha41575]

@koba1t If I understand correctly, the problem is that when you create a generator for resources, the resourceGenerator is in builtins, and needs to call accumulateTarget which is in target. But to process the kustomization file properly, accumulateTarget in target needs to call the generators in builtins, resulting in the import cycle.

We may have already discussed this, but did you try moving all the files from target (all of these files) to the builtins folder, so that it's all in the same package, to avoid the dependencies? I'm not yet sure if this is the right way to go, but I would like to know if this could potentially resolve the issue (and also help to confirm my understanding of the problem).

[koba1t]

I resolved to import cycle problem with adding member variable contain pointer of kt for PluginHelperstruct.
But I can't decide whether this is the right approach for resolving this problem.

If you have any ideas, please let me know.

[koba1t]

@natasha41575

I have a problem with fields that affect out of the resources directory scope.
I think the effect of vars and crd fields are not limited on once accumulateTarget, but any Generator needs return ResMap type result without ResAccumulator that decided by interface and ResourceGenerator return ResMap.
ResAccumulator can contain these fields' data but the ResMap struct looks like a simple struct and I think I can't store data of these fields

So, If we need to keep current behavior, we may need some other keep of these fields. Do you have any good ideas?

[koba1t]

This error message was changed due to every resources field in another directory being applied independently.

[koba1t]

No longer permitted to call the same resources two times in each components due to the change to resources was transitioned to ResMap before applying components.

[natasha41575]

@natasha41575

I have a problem with fields that affect out of the resources directory scope. I think the effect of vars and crd fields are not limited on once accumulateTarget, but any Generator needs return ResMap type result without ResAccumulator that decided by interface and ResourceGenerator return ResMap. ResAccumulator can contain these fields' data but the ResMap struct looks like a simple struct and I think I can't store data of these fields

So, If we need to keep current behavior, we may need some other keep of these fields. Do you have any good ideas?

If I understand the problem correctly, then I can think of a couple suggestions

1. I think it is OK to add crds and vars to ResMap if that appears to be the cleanest solution. To do this, I think you would need to add methods to the ResMap interface to get the crds and vars, and store them in the ResWrangler.

2. You could maybe change the generators interface to return a type ResAccumulator instead of ResMap. Then you can add the additional fields to ResAccumulator. For other generators that are not ResourceGenerator, we can leave all the other fields besides ResMap empty if needed. But I think to do this, we will need to take ResAccumulator out of api/internal, so that users can still built their own custom generators if they want to. I think we should try to avoid this though. Edited to add: If we change the generators interface, we might also have to change the transformers/validators interfaces and double check that KRM functions still work, and this might be a breaking change for legacy style functions, so maybe this solution isn't actually feasible.

3. We can define a new wrapper around ResMap, e.g. ResMapWithGlobalData that contains a ResMap and the crds and vars data.

4. Another potential option (though I'm not sure if this one will solve your issue) - we do already have a wrapper called GeneratorWithProperties that we use to store origin information. It is used to configure and run the generators:

   kustomize/api/internal/target/kusttarget.go

   Line 268 in f81765b

   var generators []*resmap.GeneratorWithProperties

   . I'm curious if it might be possible to add the crd/vars data there, and have runGenerators return them when applicable.

5. If it helps in conjunction with any of the other options, we can potentially run resourceGenerator separately from the other generators, e.g. have a separate configureResourceGenerator - separate from the configureBuiltinGenerator. This is a bit hacky though so we should try to avoid this if possible.

Of these options, I'm hoping that (3) or (4) will be feasible. (1) is okay too. (2) and (5) we should try to avoid if we can, but we can consider them if the other options aren't possible. I am also open to other solutions if you can come up with some better ideas.

[natasha41575]

@koba1t I edited my comment for (2) above - I think changing the Generators interface is actually not feasible and will have a lot of consequences, so let's try to avoid that one.

[koba1t]

Thanks for thinking of a solution and explaining it so clearly!
I'm first testing if (3) or (4) works well!

[antoooks]

Thanks for thinking of a solution and explaining it so clearly! I'm first testing if (3) or (4) works well!

Hi everyone, just a little idea of mine. How about adding crds and vars as optional field and applying Builder pattern on ResMap interface or resWrangler struct? I think putting the function on resWrangler is more appropriate since it has higher point of view on the kustomization.

func (r ResMap) withProperties(crds []kt.Crds, vars []types.Var ) ResMap {
     r.crds = crds
     r.vars = vars
     return r
}

We can call it from Factory

m, err := newResMapFromResourceSlice(ress).withProperties(crds,vars)

Alternatively, instead of adding individual optional fields like crds and vars we can make it more scalable by bundling them under an umbrella field like properties and use variadic arguments, e.g. withProperties(properties ...properties). Let me know if I don't get it correctly and I apologise if I misinterpreted the input type 🙇 .

Ideally speaking, we should separate CRDs and Vars as its own maps and make Generators return bunch of maps :)

Example implementation: https://medium.com/@reetas/clean-ways-of-adding-new-optional-fields-to-a-golang-struct-99ae2fe9719d

[natasha41575]

Thanks @antoooks! I think that's an interesting idea as well. @koba1t please feel free to explore that potential solution as well. It looks similar to my (3) above but I think @antoooks's idea is more elegant if it works.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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

[k8s-ci-robot]

PR needs rebase.

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.

[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

[koba1t]

/remove-lifecycle stale