-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Modified code for patches and patchJson6902 which will throw error if target is not found #4715
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ package krusty_test | |
import ( | ||
"testing" | ||
|
||
"github.com/stretchr/testify/assert" | ||
kusttest_test "sigs.k8s.io/kustomize/api/testutils/kusttest" | ||
) | ||
|
||
|
@@ -821,82 +822,8 @@ metadata: | |
annotations: | ||
new-key: new-value | ||
`) | ||
m := th.Run("base", th.MakeDefaultOptions()) | ||
th.AssertActualEqualsExpected(m, ` | ||
apiVersion: apps/v1 | ||
kind: Deployment | ||
metadata: | ||
labels: | ||
app: nginx | ||
name: nginx | ||
spec: | ||
template: | ||
metadata: | ||
labels: | ||
app: nginx | ||
spec: | ||
containers: | ||
- image: nginx | ||
name: nginx | ||
volumeMounts: | ||
- mountPath: /tmp/ps | ||
name: nginx-persistent-storage | ||
volumes: | ||
- emptyDir: {} | ||
name: nginx-persistent-storage | ||
- configMap: | ||
name: configmap-in-base | ||
name: configmap-in-base | ||
--- | ||
apiVersion: apps/v1 | ||
kind: Deployment | ||
metadata: | ||
labels: | ||
app: busybox | ||
name: busybox | ||
spec: | ||
template: | ||
metadata: | ||
labels: | ||
app: busybox | ||
spec: | ||
containers: | ||
- image: busybox | ||
name: busybox | ||
volumeMounts: | ||
- mountPath: /tmp/ps | ||
name: busybox-persistent-storage | ||
volumes: | ||
- emptyDir: {} | ||
name: busybox-persistent-storage | ||
- configMap: | ||
name: configmap-in-base | ||
name: configmap-in-base | ||
--- | ||
apiVersion: v1 | ||
kind: Service | ||
metadata: | ||
labels: | ||
app: nginx | ||
name: nginx | ||
spec: | ||
ports: | ||
- port: 80 | ||
selector: | ||
app: nginx | ||
--- | ||
apiVersion: v1 | ||
kind: Service | ||
metadata: | ||
labels: | ||
app: busybox | ||
name: busybox | ||
spec: | ||
ports: | ||
- port: 8080 | ||
selector: | ||
app: busybox | ||
`) | ||
err := th.RunWithErr("base", th.MakeDefaultOptions()) | ||
assert.Contains(t, err.Error(), "patches target not found for [noKind].[noVer].[noGrp]/no-match.[noNs]") | ||
} | ||
|
||
func TestExtendedPatchWithoutTarget(t *testing.T) { | ||
|
@@ -1021,82 +948,8 @@ metadata: | |
annotations: | ||
new-key: new-value | ||
`) | ||
m := th.Run("base", th.MakeDefaultOptions()) | ||
th.AssertActualEqualsExpected(m, ` | ||
apiVersion: apps/v1 | ||
kind: Deployment | ||
metadata: | ||
labels: | ||
app: nginx | ||
name: nginx | ||
spec: | ||
template: | ||
metadata: | ||
labels: | ||
app: nginx | ||
spec: | ||
containers: | ||
- image: nginx | ||
name: nginx | ||
volumeMounts: | ||
- mountPath: /tmp/ps | ||
name: nginx-persistent-storage | ||
volumes: | ||
- emptyDir: {} | ||
name: nginx-persistent-storage | ||
- configMap: | ||
name: configmap-in-base | ||
name: configmap-in-base | ||
--- | ||
apiVersion: apps/v1 | ||
kind: Deployment | ||
metadata: | ||
labels: | ||
app: busybox | ||
name: busybox | ||
spec: | ||
template: | ||
metadata: | ||
labels: | ||
app: busybox | ||
spec: | ||
containers: | ||
- image: busybox | ||
name: busybox | ||
volumeMounts: | ||
- mountPath: /tmp/ps | ||
name: busybox-persistent-storage | ||
volumes: | ||
- emptyDir: {} | ||
name: busybox-persistent-storage | ||
- configMap: | ||
name: configmap-in-base | ||
name: configmap-in-base | ||
--- | ||
apiVersion: v1 | ||
kind: Service | ||
metadata: | ||
labels: | ||
app: nginx | ||
name: nginx | ||
spec: | ||
ports: | ||
- port: 80 | ||
selector: | ||
app: nginx | ||
--- | ||
apiVersion: v1 | ||
kind: Service | ||
metadata: | ||
labels: | ||
app: busybox | ||
name: busybox | ||
spec: | ||
ports: | ||
- port: 8080 | ||
selector: | ||
app: busybox | ||
`) | ||
err := th.RunWithErr("base", th.MakeDefaultOptions()) | ||
assert.Contains(t, err.Error(), "patches target not found for [noKind].[noVer].[noGrp]/no-match.[noNs]") | ||
} | ||
|
||
func TestExtendedPatchMultiplePatchOverlapping(t *testing.T) { | ||
|
@@ -1213,3 +1066,44 @@ spec: | |
app: busybox | ||
`) | ||
} | ||
|
||
func TestTargetMissingPatchJson6902Error(t *testing.T) { | ||
th := kusttest_test.MakeHarness(t) | ||
makeCommonFileForExtendedPatchTest(th) | ||
th.WriteK("base", ` | ||
resources: | ||
- servicemonitor.yaml | ||
patchesJson6902: | ||
- target: | ||
group: monitoring.coreos.com | ||
kind: ServiceMonitor | ||
name: starboard-exporter | ||
namespace: starboard | ||
version: v2 | ||
path: patch.0.yaml | ||
`) | ||
th.WriteF("base/servicemonitor.yaml", ` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we use the existing files in |
||
apiVersion: monitoring.coreos.com/v1 | ||
kind: ServiceMonitor | ||
metadata: | ||
labels: | ||
app: starboard-exporter | ||
name: starboard-exporter | ||
namespace: starboard | ||
spec: | ||
endpoints: | ||
- path: /metrics | ||
port: metrics | ||
selector: | ||
matchLabels: | ||
app.kubernetes.io/instance: starboard-exporter | ||
app.kubernetes.io/name: starboard-exporter | ||
`) | ||
th.WriteF("base/patch.0.yaml", ` | ||
- op: add | ||
path: /metadata/labels/release | ||
value: kube-prometheus-stack | ||
`) | ||
err := th.RunWithErr("base", th.MakeDefaultOptions()) | ||
assert.Contains(t, err.Error(), "patchesJson6902 target not found for ServiceMonitor.v2.monitoring.coreos.com/starboard-exporter.starboard") | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -100,6 +100,11 @@ func (p *plugin) transformStrategicMerge(m resmap.ResMap, patch *resource.Resour | |
if err != nil { | ||
return err | ||
} | ||
|
||
if p.Options["allowNoTargetMatch"] { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is close. Instead, we want to emit this warning if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. totally agree. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated as below:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see your comment, but this doesn't seem to be reflected in the code? Can you commit your changes? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @annasong20 - We can create a PatchTransformer struct under There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good question! I only need |
||
log.Println("Warning: patches target not found for Target") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we print to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated as below:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see this change in the code. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we print for users the "Target" that we're looking for? As a user with a complex Kustomization, I'd be confused and need to debug which patch target is missing. |
||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We still need to return an error if |
||
|
||
return m.ApplySmPatch(resource.MakeIdSet(selected), patch) | ||
} | ||
|
||
|
@@ -113,6 +118,11 @@ func (p *plugin) transformJson6902(m resmap.ResMap, patch jsonpatch.Patch) error | |
if err != nil { | ||
return err | ||
} | ||
|
||
if p.Options["allowNoTargetMatch"] { | ||
log.Println("Warning: patches target not found for Target") | ||
} | ||
Comment on lines
+122
to
+124
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated same as above. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't see the change in code. |
||
|
||
for _, res := range resources { | ||
res.StorePreviousId() | ||
internalAnnotations := kioutil.GetInternalAnnotations(&res.RNode) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't seem to have tests for
allowNoTargetMatch
patches
field