Emit a warning on build when deprecated fields are used #4723

koba1t · 2022-07-25T20:24:01Z

koba1t · 2022-07-25T20:24:54Z

natasha41575

Looks pretty good! A few small comments. Thanks for picking this up.

natasha41575 · 2022-07-28T20:29:35Z

api/types/kustomization_test.go

+		k    Kustomization
+		want *[]string
+	}{
+		// TODO: Add test cases.


is this TODO still needed?

A few tests are already added, and It isn't needed.
I'll delete this comment.

natasha41575 · 2022-07-28T20:30:51Z

api/types/kustomization.go

+const (
+	deprecatedBaseWarningMessage           = "# Warning: bases was deprecated, Please use resources or try `kustomize edit fix`."
+	deprecatedPatchesJson6902Message       = "# Warning: patchesJson6902 is deprecated, Please use patches or try `kustomize edit fix`."
+	deprecatedPatchesStrategicMergeMessage = "# Warning: bases is patchesStrategicMerge, Please use patches or try `kustomize edit fix`."


It looks like patchesStrategicMerge isn't handled by kustomize edit fix at the moment, so I think we should either wait for that change to go in first, or we need to change this error message to not mention kustomize edit fix yet.

I fixed this message.

So, I add edit fix function with #4733
If you have time, Could you watch this?

natasha41575 · 2022-07-28T20:31:05Z

api/types/kustomization.go

+const (
+	deprecatedBaseWarningMessage           = "# Warning: bases was deprecated, Please use resources or try `kustomize edit fix`."
+	deprecatedPatchesJson6902Message       = "# Warning: patchesJson6902 is deprecated, Please use patches or try `kustomize edit fix`."
+	deprecatedPatchesStrategicMergeMessage = "# Warning: bases is patchesStrategicMerge, Please use patches or try `kustomize edit fix`."


Suggested change

deprecatedPatchesStrategicMergeMessage = "# Warning: bases is patchesStrategicMerge, Please use patches or try `kustomize edit fix`."

deprecatedPatchesStrategicMergeMessage = "# Warning: patchesStrategicMerge is deprecated, Please use patches or try `kustomize edit fix`."

Thanks! I fix.

natasha41575 · 2022-07-28T20:31:52Z

api/types/kustomization.go

@@ -172,6 +172,31 @@ type Kustomization struct {
 	BuildMetadata []string `json:"buildMetadata,omitempty" yaml:"buildMetadata,omitempty"`
 }

+const (
+	deprecatedBaseWarningMessage           = "# Warning: bases was deprecated, Please use resources or try `kustomize edit fix`."


nit: quote the field names (here, and in the other messages as well)

# Warning: bases was deprecated, please use 'resources' or try `kustomize edit fix`.

I fixed it.

k8s-ci-robot · 2022-07-29T12:49:13Z

@koba1t: 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.

netlify · 2022-07-29T12:49:34Z

✅ Deploy Preview for kubernetes-sigs-kustomize canceled.

Name	Link
🔨 Latest commit	`438758b`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kustomize/deploys/62e3d749a7928e00089e5dbb

koba1t · 2022-07-29T17:26:54Z

hi @natasha41575
I think I completed fixing these.
Could you recheck it?

natasha41575 · 2022-07-29T18:03:07Z

api/types/kustomization.go

+	deprecatedBaseWarningMessage           = "# Warning: bases was deprecated, Please use 'resources' or try `kustomize edit fix`."
+	deprecatedPatchesJson6902Message       = "# Warning: patchesJson6902 is deprecated, Please use 'patches' or try `kustomize edit fix`."
+	deprecatedPatchesStrategicMergeMessage = "# Warning: patchesStrategicMerge is deprecated, Please use 'patches'."
+	deprecatedVarsMessage                  = "# Warning: vars is deprecated, Please use 'replacements'."


@KnVerey Would you like to take a look a these warning messages before we merge?

Please change the commas after "deprecated" to periods.

Please quote the names of the old fields, to match the names of the new.

Since the point of kustomize edit fix is to do these updates for you, it isn't really an alternative to using the new fields. How about Warning: 'X' is deprecated. Please use 'Y' instead. Run 'kustomize edit fix' to update your Kustomization automatically..

For replacements, we can add something like "(EXPERIMENTAL)" to the edit fix part.

If this merges before add edit fix for patchesStrategicMerge to patches #4733, that PR will need to update the message.

Hi @KnVerey
I think I completed to fix from your comment.

If this merges before #4733, that PR will need to update the message.

I think it is better to merge first to #4733 if I can.
I added a warning message to patchesStrategicMerge in this PR.

@natasha41575
Would you review #4733 before merging this PR?

api/types/kustomization.go

KnVerey · 2022-08-10T19:19:19Z

api/types/kustomization.go

+	deprecatedBaseWarningMessage           = "# Warning: bases was deprecated, Please use 'resources' or try `kustomize edit fix`."
+	deprecatedPatchesJson6902Message       = "# Warning: patchesJson6902 is deprecated, Please use 'patches' or try `kustomize edit fix`."
+	deprecatedPatchesStrategicMergeMessage = "# Warning: patchesStrategicMerge is deprecated, Please use 'patches'."
+	deprecatedVarsMessage                  = "# Warning: vars is deprecated, Please use 'replacements'."


Please change the commas after "deprecated" to periods.

Please quote the names of the old fields, to match the names of the new.

Since the point of kustomize edit fix is to do these updates for you, it isn't really an alternative to using the new fields. How about Warning: 'X' is deprecated. Please use 'Y' instead. Run 'kustomize edit fix' to update your Kustomization automatically..

For replacements, we can add something like "(EXPERIMENTAL)" to the edit fix part.

If this merges before add edit fix for patchesStrategicMerge to patches #4733, that PR will need to update the message.

api/types/kustomization.go

KnVerey · 2022-08-12T18:01:31Z

api/types/kustomization.go

+const (
+	deprecatedWarningToRunEditFix              = "# Warning: Run 'kustomize edit fix' to update your Kustomization automatically."
+	deprecatedWarningToRunEditFixExperimential = "# Warning: [EXPERIMENTAL]Run 'kustomize edit fix' to update your Kustomization automatically."
+	deprecatedBaseWarningMessage               = "# Warning: 'bases' is deprecated. Please use 'resources' instead." + "\n" + deprecatedWarningToRunEditFix


since this is a single problem, I would prefer to have it printed on one line, without the second "warning" text

I restore it to one-line warning messages.

api/types/kustomization.go

KnVerey · 2022-08-12T18:13:14Z

api/internal/target/kusttarget.go

@@ -69,6 +70,15 @@ func (kt *KustTarget) Load() error {
 	if err != nil {
 		return err
 	}
+
+	// show warning message when using deprecated fields.
+	warningMessages := k.CheckDeprecatedFields()


Are we sure this is going to work, given that FixKustomizationPreUnmarshalling above mutates the file to accommodate some of the deprecated fields?

Looks like FixKustomizationPreUnmarshalling is fixing for raw []byte data before unmarshaling to yaml.

kustomize/api/types/fix.go

Lines 12 to 32 in f61b075

// FixKustomizationPreUnmarshalling modifies the raw data

// before marshalling - e.g. changes old field names to

// new field names.

func FixKustomizationPreUnmarshalling(data []byte) ([]byte, error) {

deprecatedFieldsMap := map[string]string{

"imageTags:": "images:",

}

for oldname, newname := range deprecatedFieldsMap {

pattern := regexp.MustCompile(oldname)

data = pattern.ReplaceAll(data, []byte(newname))

}

doLegacy, err := useLegacyPatch(data)

if err != nil {

return nil, err

}

if doLegacy {

pattern := regexp.MustCompile("patches:")

data = pattern.ReplaceAll(data, []byte("patchesStrategicMerge:"))

}

return data, nil

}

I think this function doesn't affect some of the deprecated fields in Kustomization struct.

Interestingly, this changes imageTags to images, which is a deprecation I wasn't even aware of. Also, it makes it seem like the field called patchesStrategicMerge was originally called patches, and we later re-introduced patches with a different meaning. 🤦

Ideally, I think we should warn about imageTags too, so we can get rid of FixKustomizationPreUnmarshalling along with all the deprecated fields.

Also, I wonder if folks using the super old version of patches would get a weird warning about patchesStrategicMerge (which they aren't using) because of this method. Can we check this please?

Ideally, I think we should warn about imageTags too, so we can get rid of FixKustomizationPreUnmarshalling along with all the deprecated fields.

I think it is hard to delete the FixKustomizationPreUnmarshalling function while we support the super old version of patches.
Because super old patches (It is called patchesStrategicMerge today) have a different struct to today's patches field.
I think we will get an error when unmarshaling []byte to Kustomization struct if we delete the pre unmarshalling process.

kustomize/api/types/kustomization.go

Lines 66 to 76 in 4fc0249

PatchesStrategicMerge []PatchStrategicMerge `json:"patchesStrategicMerge,omitempty" yaml:"patchesStrategicMerge,omitempty"`

// JSONPatches is a list of JSONPatch for applying JSON patch.

// Format documented at https://tools.ietf.org/html/rfc6902

// and http://jsonpatch.com

PatchesJson6902 []Patch `json:"patchesJson6902,omitempty" yaml:"patchesJson6902,omitempty"`

// Patches is a list of patches, where each one can be either a

// Strategic Merge Patch or a JSON patch.

// Each patch can be applied to multiple target objects.

Patches []Patch `json:"patches,omitempty" yaml:"patches,omitempty"`

kustomize/api/types/patchstrategicmerge.go

Line 9 in 4fc0249

type PatchStrategicMerge string

kustomize/api/types/patch.go

Lines 8 to 24 in 4fc0249

// Patch represent either a Strategic Merge Patch or a JSON patch

// and its targets.

// The content of the patch can either be from a file

// or from an inline string.

type Patch struct {

// Path is a relative file path to the patch file.

Path string `json:"path,omitempty" yaml:"path,omitempty"`

// Patch is the content of a patch.

Patch string `json:"patch,omitempty" yaml:"patch,omitempty"`

// Target points to the resources that the patch is applied to

Target *Selector `json:"target,omitempty" yaml:"target,omitempty"`

// Options is a list of options for the patch

Options map[string]bool `json:"options,omitempty" yaml:"options,omitempty"`

}

Also, I wonder if folks using the super old version of patches would get a weird warning about patchesStrategicMerge (which they aren't using) because of this method. Can we check this please?

That is right. If we use very old style patches, the warning message of patchesStrategicMerge appear...

api/types/kustomization.go

KnVerey · 2022-08-12T18:24:09Z

api/types/kustomization.go

@@ -116,7 +119,7 @@ type Kustomization struct {
 	// CRDs themselves are not modified.
 	Crds []string `json:"crds,omitempty" yaml:"crds,omitempty"`

-	// Deprecated.
+	// Deprecated: Replace with the Resources.


Instead of adding a line, add "Deprecated:" to the existing message below.

I fixed to below line.
https://github.com/kubernetes-sigs/kustomize/pull/4723/files#diff-fa94909235803d7e221a81bbf24d0005b59062f6f4b80110a1d2b390919379c7R122

KnVerey · 2022-08-12T18:27:04Z

I think it is better to merge first to #4733 if I can.
I added a warning message to patchesStrategicMerge in this PR.

I agree with you. So we don't forget that that order is required:

/hold for #4733 to merge first

koba1t · 2022-08-12T19:28:27Z

Hi @KnVerey
I think I completed to fix from your comment.
If you have time, could you please recheck again?

koba1t · 2022-11-07T18:42:09Z

/remove-hold

natasha41575 · 2022-11-16T16:25:35Z

Nice!

$ kustomize build > out
# Warning: 'bases' is deprecated. Please use 'resources' instead. Run 'kustomize edit fix' to update your Kustomization automatically.
# Warning: 'patchesStrategicMerge' is deprecated. Please use 'patches' instead. Run 'kustomize edit fix' to update your Kustomization automatically.

/approve

k8s-ci-robot · 2022-11-16T16:25:44Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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~~ [natasha41575]

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

natasha41575 · 2022-11-16T16:26:13Z

/lgtm

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 25, 2022

k8s-ci-robot requested review from KnVerey and natasha41575 July 25, 2022 20:24

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 25, 2022

k8s-ci-robot assigned natasha41575 Jul 25, 2022

natasha41575 reviewed Jul 28, 2022

View reviewed changes

natasha41575 reviewed Jul 29, 2022

View reviewed changes

koba1t force-pushed the emit_a_warning_when_deprecated_fields_are_used branch 3 times, most recently from e12bf75 to 90231f4 Compare August 2, 2022 15:41

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 2, 2022

KnVerey reviewed Aug 10, 2022

View reviewed changes

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 11, 2022

koba1t force-pushed the emit_a_warning_when_deprecated_fields_are_used branch from 6b200a3 to 16a10df Compare August 11, 2022 18:30

KnVerey reviewed Aug 12, 2022

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 12, 2022

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 26, 2022

Emit a warning when deprecated fields are used

21ee7f7

koba1t force-pushed the emit_a_warning_when_deprecated_fields_are_used branch from 4c7bfde to 21ee7f7 Compare September 1, 2022 23:06

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 1, 2022

natasha41575 mentioned this pull request Oct 21, 2022

add edit fix for patchesStrategicMerge to patches #4733

Merged

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 7, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 16, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 16, 2022

k8s-ci-robot merged commit b20e611 into kubernetes-sigs:master Nov 16, 2022

koba1t deleted the emit_a_warning_when_deprecated_fields_are_used branch November 22, 2022 17:29

koba1t mentioned this pull request Dec 9, 2022

Remove fix kustomization step before Unmarshalling the yaml structure #4923

Merged

natasha41575 mentioned this pull request Mar 14, 2023

Update kubectl kustomize to kyaml/v0.14.1, cmd/config/v0.11.1, api/v0… kubernetes/kubernetes#116598

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Emit a warning on build when deprecated fields are used #4723

Emit a warning on build when deprecated fields are used #4723

koba1t commented Jul 25, 2022

koba1t commented Jul 25, 2022

natasha41575 left a comment

natasha41575 Jul 28, 2022

koba1t Jul 29, 2022

natasha41575 Jul 28, 2022

koba1t Jul 29, 2022

natasha41575 Jul 28, 2022

koba1t Jul 29, 2022

natasha41575 Jul 28, 2022

koba1t Jul 29, 2022

k8s-ci-robot commented Jul 29, 2022

netlify bot commented Jul 29, 2022 •

edited

Loading

koba1t commented Jul 29, 2022

natasha41575 Jul 29, 2022

KnVerey Aug 10, 2022

koba1t Aug 11, 2022 •

edited

Loading

KnVerey Aug 10, 2022

KnVerey Aug 12, 2022

koba1t Aug 12, 2022

KnVerey Aug 12, 2022

koba1t Aug 12, 2022

KnVerey Aug 12, 2022

koba1t Aug 12, 2022 •

edited

Loading

koba1t Aug 12, 2022 •

edited

Loading

KnVerey Aug 12, 2022

koba1t Aug 12, 2022

KnVerey commented Aug 12, 2022

koba1t commented Aug 12, 2022

koba1t commented Nov 7, 2022

natasha41575 commented Nov 16, 2022

k8s-ci-robot commented Nov 16, 2022

natasha41575 commented Nov 16, 2022

	deprecatedPatchesStrategicMergeMessage = "# Warning: bases is patchesStrategicMerge, Please use patches or try `kustomize edit fix`."
	deprecatedPatchesStrategicMergeMessage = "# Warning: patchesStrategicMerge is deprecated, Please use patches or try `kustomize edit fix`."

	// FixKustomizationPreUnmarshalling modifies the raw data
	// before marshalling - e.g. changes old field names to
	// new field names.
	func FixKustomizationPreUnmarshalling(data []byte) ([]byte, error) {
	deprecatedFieldsMap := map[string]string{
	"imageTags:": "images:",
	}
	for oldname, newname := range deprecatedFieldsMap {
	pattern := regexp.MustCompile(oldname)
	data = pattern.ReplaceAll(data, []byte(newname))
	}
	doLegacy, err := useLegacyPatch(data)
	if err != nil {
	return nil, err
	}
	if doLegacy {
	pattern := regexp.MustCompile("patches:")
	data = pattern.ReplaceAll(data, []byte("patchesStrategicMerge:"))
	}
	return data, nil
	}

	PatchesStrategicMerge []PatchStrategicMerge `json:"patchesStrategicMerge,omitempty" yaml:"patchesStrategicMerge,omitempty"`

	// JSONPatches is a list of JSONPatch for applying JSON patch.
	// Format documented at https://tools.ietf.org/html/rfc6902
	// and http://jsonpatch.com
	PatchesJson6902 []Patch `json:"patchesJson6902,omitempty" yaml:"patchesJson6902,omitempty"`

	// Patches is a list of patches, where each one can be either a
	// Strategic Merge Patch or a JSON patch.
	// Each patch can be applied to multiple target objects.
	Patches []Patch `json:"patches,omitempty" yaml:"patches,omitempty"`

	// Patch represent either a Strategic Merge Patch or a JSON patch
	// and its targets.
	// The content of the patch can either be from a file
	// or from an inline string.
	type Patch struct {
	// Path is a relative file path to the patch file.
	Path string `json:"path,omitempty" yaml:"path,omitempty"`

	// Patch is the content of a patch.
	Patch string `json:"patch,omitempty" yaml:"patch,omitempty"`

	// Target points to the resources that the patch is applied to
	Target *Selector `json:"target,omitempty" yaml:"target,omitempty"`

	// Options is a list of options for the patch
	Options map[string]bool `json:"options,omitempty" yaml:"options,omitempty"`
	}

Emit a warning on build when deprecated fields are used #4723

Emit a warning on build when deprecated fields are used #4723

Conversation

koba1t commented Jul 25, 2022

koba1t commented Jul 25, 2022

natasha41575 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 29, 2022

netlify bot commented Jul 29, 2022 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kustomize canceled.

koba1t commented Jul 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koba1t Aug 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koba1t Aug 12, 2022 • edited Loading

Choose a reason for hiding this comment

koba1t Aug 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KnVerey commented Aug 12, 2022

koba1t commented Aug 12, 2022

koba1t commented Nov 7, 2022

natasha41575 commented Nov 16, 2022

k8s-ci-robot commented Nov 16, 2022

natasha41575 commented Nov 16, 2022

netlify bot commented Jul 29, 2022 •

edited

Loading

koba1t Aug 11, 2022 •

edited

Loading

koba1t Aug 12, 2022 •

edited

Loading

koba1t Aug 12, 2022 •

edited

Loading