ResId.Equals usable for VariableRef. #1327
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
/assign @Liujingfang1 |
k8s-ci-robot
added
the
size/L
jbrette
force-pushed
the
residrequals-variables
branch
from
3d9e3af to
f56c5df
Compare
jbrette
force-pushed
the
residrequals-variables
branch
7 times, most recently
from
5e3bb4f to
08c001f
Compare
|
@Liujingfang1 Gentle push of the PR. Same thing. No actual code change, just a comment to warn a contributor not to change GkvnEqual to Equal until change are done to the definition of the variableRef. Added a test covering for issue 1298. |
|
Seconded on the review request @Liujingfang1. Deeply appreciate all your work on kustomize, not trying to be a pest, it would just be wonderful to see a bit of movement on this and @jbrette's associated PRs around "diamond importing". |
jbrette
force-pushed
the
residrequals-variables
branch
from
08c001f to
b071857
Compare
|
@jbrette @tkellen Will take a look at this change today. For the |
|
@Liujingfang1 I just rebased this PR. It actually does not change the code and will allow the team to track the eventual change of the variable declaration will solve issue #1298. Solving the issue 1298 itself will required some work. |
| // https://github.com/kubernetes-sigs/kustomize/issues/1298 | ||
| const namespaceNeedInVarMyApp string = ` | ||
| resources: | ||
| - elasticsearch-test-service.yaml |
There was a problem hiding this comment.
This test case can be changed to a different layout
- base with var
- overlay for test namespace
- overlay for dev namespace
- both
This layout currently fails, but I think we should fix it.
There was a problem hiding this comment.
@Liujingfang1 Don't completely understand the request. The PR has two tests. The first one is matching your "base with var". The second called workaround is matching "both".
There was a problem hiding this comment.
This layout is a different use case that came into my mind when reviewing this PR. It is not due to the same root cause, but I think we need to fix it.
jbrette
force-pushed
the
residrequals-variables
branch
from
b071857 to
1344f1e
Compare
- Namespace need objRef field in variable declaration - Add namespace conflict test for variables The replacement of ResId.GkvnEquals reference by ResId.Equals highligthed the fact it is no possible yet when looking for variable targets because the namespace field is not allowed yet. This commit adds two tests to the namespaces_test.go regarding that use case.
jbrette
force-pushed
the
residrequals-variables
branch
from
1344f1e to
0bec7b9
Compare
|
@Liujingfang1 Please merge this PR first. |
|
@tkellen @Liujingfang1 @monopole We are just getting there. The current PR is just pushing to conflict resolution to the SMPConflictDetector instead of just failing because of conflict in var names or resource Id. So if your are trying to mix patches which are "adding things", it will work. Hence kustomize is just better, not perfect but it can deal with more cases. So we are just better than before. The real fix will come the day we are able to leverage the ThreeWayMerge from SMP, If you check in details, the PR is compliant with 1292. The idea is that if the merge could not happen automatically, it would ask for support in the kustomization.yaml in order to solve the merge conflct. |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jbrette, Liujingfang1 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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
@jbrette Should this landing fix the usecase I defined in #1316 (comment)? |
As per request, splitting big PR into smaller ones:
This PR address the issue #1298 . We can't change ResId.GkvnEquals to ResId.Equals yet because the namespace is not part of the variable declaration.
Added a test case demonstrating the umbiguaus variable error causes by that problem