Allow diamond structure of kustomize import #1253
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jbrette The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
size/L
|
/assign @Liujingfang1 |
| // name already exists | ||
| if !reflect.DeepEqual(v, vs.set[v.Name]) { | ||
| return fmt.Errorf( | ||
| "var '%s' already encountered", v.Name) |
There was a problem hiding this comment.
I think this is what vs.Contains does.
There was a problem hiding this comment.
No vs.Contains is using the name of the variable. The DeepEquals allows to detect that we are trying to insert a new variable with the same name but a different definition.
jbrette
force-pushed
the
diamond
branch
3 times, most recently
from
c846b09 to
387029b
Compare
jbrette
changed the title
k8s-ci-robot
added
the
do-not-merge/work-in-progress
jbrette
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
@Liujingfang1 @monopole Extended diamond test showing simple sharing of a common folder between two component1 and component2 before beeing aggregate together in myapp. |
jbrette
force-pushed
the
diamond
branch
4 times, most recently
from
9cdbfb1 to
0e1242d
Compare
- This situation happens in project which try to aggregate multiple components in different folders sharing the same base folder. - Improve Resource Append to allow diamond include. - Improve VarSet Merge method to allow diamond include.
- Variables not involving nameprefix are resolved properly. - variables resolved properly for first pod. - Still quite a lot of work to get the variable resolved properly for Pod2.
This commit demonstrates how a common resource can be included accross different components.
k8s-ci-robot
added
size/XL
|
I agree with the spirit of this PR, but not the implementation. The PR didn't require changes to many tests, but since i wrote a good fraction of them, they assume that the resmap disallows Id collisions, and so don't test for it. Allowing it will cause new behavior and new bugs. We can't just toss in a big change like this without a flag that enables it, and a coherent testing and example strategy. Started an issue here: #1292 At the time of writing this note, my understanding is that the particular issue (#1251) this PR wants to address can be solved by writing different kustomization files. |
k8s-ci-robot
added
the
needs-rebase
|
@jbrette: 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. |
|
PR has been merged and designed explicitly to generate merge conflict. |
|
#1291 added a test to assure the resmap class invariant (unique Ids) was protected, so that a PR that violated that assumption would be flagged. It's a dangerous assumption to violate, and we don't have enough coverage on it. This PR brought that fact to my attention, that is all. Thanks for the effort. |
components in different folders sharing the same base folder.
It addresses the following issues:
Support composition in kustomize #1251
and
Error when composing several identical bases that use the same var: "var ... already encountered" #1248