Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
Nov 16, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 10 additions & 0 deletions api/internal/target/kusttarget.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package target
import (
"encoding/json"
"fmt"
"os"
"strings"

"github.com/pkg/errors"
Expand Down Expand Up @@ -69,6 +70,15 @@ func (kt *KustTarget) Load() error {
if err != nil {
return err
}

// show warning message when using deprecated fields.
warningMessages := k.CheckDeprecatedFields()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@koba1t koba1t Aug 12, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"`
}

Copy link
Member Author

@koba1t koba1t Aug 12, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

if warningMessages != nil {
for _, msg := range *warningMessages {
fmt.Fprintf(os.Stderr, "%v\n", msg)
}
}

k.FixKustomizationPostUnmarshalling()
errs := k.EnforceFields()
if len(errs) > 0 {
Expand Down
37 changes: 34 additions & 3 deletions api/types/kustomization.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,12 +59,14 @@ type Kustomization struct {
// CommonAnnotations to add to all objects.
CommonAnnotations map[string]string `json:"commonAnnotations,omitempty" yaml:"commonAnnotations,omitempty"`

// Deprecated: Use the Patches field instead, which provides a superset of the functionality of PatchesStrategicMerge.
// PatchesStrategicMerge specifies the relative path to a file
// containing a strategic merge patch. Format documented at
// https://github.com/kubernetes/community/blob/master/contributors/devel/sig-api-machinery/strategic-merge-patch.md
// URLs and globs are not supported.
PatchesStrategicMerge []PatchStrategicMerge `json:"patchesStrategicMerge,omitempty" yaml:"patchesStrategicMerge,omitempty"`

// Deprecated: Use the Patches field instead, which provides a superset of the functionality of JSONPatches.
// JSONPatches is a list of JSONPatch for applying JSON patch.
// Format documented at https://tools.ietf.org/html/rfc6902
// and http://jsonpatch.com
Expand All @@ -88,6 +90,7 @@ type Kustomization struct {
// specification. This can also be done with a patch.
Replicas []Replica `json:"replicas,omitempty" yaml:"replicas,omitempty"`

// Deprecated: Vars will be removed in future release. Migrate to Replacements instead.
// Vars allow things modified by kustomize to be injected into a
// kubernetes object specification. A var is a name (e.g. FOO) associated
// with a field in a specific resource instance. The field must
Expand Down Expand Up @@ -116,9 +119,7 @@ type Kustomization struct {
// CRDs themselves are not modified.
Crds []string `json:"crds,omitempty" yaml:"crds,omitempty"`

// Deprecated.
// Anything that would have been specified here should
// be specified in the Resources field instead.
// Deprecated: Anything that would have been specified here should be specified in the Resources field instead.
Bases []string `json:"bases,omitempty" yaml:"bases,omitempty"`

//
Expand Down Expand Up @@ -172,6 +173,33 @@ type Kustomization struct {
BuildMetadata []string `json:"buildMetadata,omitempty" yaml:"buildMetadata,omitempty"`
}

const (
deprecatedWarningToRunEditFix = "Run 'kustomize edit fix' to update your Kustomization automatically."
deprecatedWarningToRunEditFixExperimential = "[EXPERIMENTAL] Run 'kustomize edit fix' to update your Kustomization automatically."
deprecatedBaseWarningMessage = "# Warning: 'bases' is deprecated. Please use 'resources' instead." + " " + deprecatedWarningToRunEditFix
deprecatedPatchesJson6902Message = "# Warning: 'patchesJson6902' is deprecated. Please use 'patches' instead." + " " + deprecatedWarningToRunEditFix
deprecatedPatchesStrategicMergeMessage = "# Warning: 'patchesStrategicMerge' is deprecated. Please use 'patches' instead." + " " + deprecatedWarningToRunEditFix
deprecatedVarsMessage = "# Warning: 'vars' is deprecated. Please use 'replacements' instead." + " " + deprecatedWarningToRunEditFixExperimential
)

// CheckDeprecatedFields check deprecated field is used or not.
func (k *Kustomization) CheckDeprecatedFields() *[]string {
var warningMessages []string
if k.Bases != nil {
warningMessages = append(warningMessages, deprecatedBaseWarningMessage)
}
if k.PatchesJson6902 != nil {
warningMessages = append(warningMessages, deprecatedPatchesJson6902Message)
}
if k.PatchesStrategicMerge != nil {
warningMessages = append(warningMessages, deprecatedPatchesStrategicMergeMessage)
}
if k.Vars != nil {
warningMessages = append(warningMessages, deprecatedVarsMessage)
}
return &warningMessages
}

// FixKustomizationPostUnmarshalling fixes things
// like empty fields that should not be empty, or
// moving content of deprecated fields to newer
Expand All @@ -187,8 +215,11 @@ func (k *Kustomization) FixKustomizationPostUnmarshalling() {
k.APIVersion = KustomizationVersion
}
}

// 'bases' field was deprecated in favor of the 'resources' field.
k.Resources = append(k.Resources, k.Bases...)
k.Bases = nil

for i, g := range k.ConfigMapGenerator {
if g.EnvSource != "" {
k.ConfigMapGenerator[i].EnvSources =
Expand Down
60 changes: 60 additions & 0 deletions api/types/kustomization_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,66 @@ func fixKustomizationPostUnmarshallingCheck(k, e *Kustomization) bool {
k.Bases == nil
}

func TestKustomization_CheckDeprecatedFields(t *testing.T) {
tests := []struct {
name string
k Kustomization
want *[]string
}{
{
name: "using_bases",
k: Kustomization{
Bases: []string{"base"},
},
want: &[]string{deprecatedBaseWarningMessage},
},
{
name: "usingPatchesJson6902",
k: Kustomization{
PatchesJson6902: []Patch{},
},
want: &[]string{deprecatedPatchesJson6902Message},
},
{
name: "usingPatchesStrategicMerge",
k: Kustomization{
PatchesStrategicMerge: []PatchStrategicMerge{},
},
want: &[]string{deprecatedPatchesStrategicMergeMessage},
},
{
name: "usingVar",
k: Kustomization{
Vars: []Var{},
},
want: &[]string{deprecatedVarsMessage},
},
{
name: "usingAll",
k: Kustomization{
Bases: []string{"base"},
PatchesJson6902: []Patch{},
PatchesStrategicMerge: []PatchStrategicMerge{},
Vars: []Var{},
},
want: &[]string{
deprecatedBaseWarningMessage,
deprecatedPatchesJson6902Message,
deprecatedPatchesStrategicMergeMessage,
deprecatedVarsMessage,
},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
k := tt.k
if got := k.CheckDeprecatedFields(); !reflect.DeepEqual(got, tt.want) {
t.Errorf("Kustomization.CheckDeprecatedFields() = %v, want %v", got, tt.want)
}
})
}
}

func TestFixKustomizationPostUnmarshalling(t *testing.T) {
var k Kustomization
k.Bases = append(k.Bases, "foo")
Expand Down