From 8aed0e9b16603cb0736cb57b879241c2aad7df3a Mon Sep 17 00:00:00 2001 From: John Ryan Date: Thu, 16 Sep 2021 07:09:05 -0700 Subject: [PATCH] Fit and finish from review - extract meta type names as constants - fix slice append gotcha: when creating a new slice, copy from old first (i.e. copyAndAppendMeta()) - various renames and inlining to improve clarity --- hack/test-all-minikube-local-registry.sh | 3 +- pkg/kbld/cmd/resolve.go | 11 ++--- pkg/kbld/config/config.go | 6 +-- pkg/kbld/config/image_meta.go | 57 +++++++++++++++++------- pkg/kbld/image/built.go | 4 +- pkg/kbld/image/preresolved.go | 11 +++-- pkg/kbld/image/resolved.go | 2 +- pkg/kbld/image/tagged.go | 2 +- 8 files changed, 61 insertions(+), 35 deletions(-) diff --git a/hack/test-all-minikube-local-registry.sh b/hack/test-all-minikube-local-registry.sh index 4890b663..0ac66d33 100755 --- a/hack/test-all-minikube-local-registry.sh +++ b/hack/test-all-minikube-local-registry.sh @@ -5,7 +5,8 @@ set -e -x -u export KBLD_E2E_DOCKERHUB_USERNAME=minikube-tests export KBLD_E2E_DOCKERHUB_HOSTNAME=$(minikube ip):30777 # uncomment to disable stress tests -# export KBLD_E2E_SKIP_STRESS_TESTS=true +export KBLD_E2E_SKIP_STRESS_TESTS=true +export KBLD_E2E_SKIP_WHEN_HTTP_REGISTRY=true kapp deploy -a reg -f test/e2e/assets/minikube-local-registry.yml -y diff --git a/pkg/kbld/cmd/resolve.go b/pkg/kbld/cmd/resolve.go index 91779680..ee134ae7 100644 --- a/pkg/kbld/cmd/resolve.go +++ b/pkg/kbld/cmd/resolve.go @@ -246,11 +246,9 @@ func (o *ResolveOptions) emitLockOutput(conf ctlconf.Conf, resolvedImages *Proce }, } for _, urlImagePair := range resolvedImages.All() { - anns := o.imgpkgLockAnnotations(urlImagePair) - iLock.Images = append(iLock.Images, lockconfig.ImageRef{ Image: urlImagePair.Image.URL, - Annotations: anns, + Annotations: o.imgpkgLockAnnotations(urlImagePair), }) } return iLock.WriteToPath(o.ImgpkgLockOutput) @@ -263,13 +261,12 @@ func (o *ResolveOptions) imgpkgLockAnnotations(i ProcessedImageItem) map[string] anns := map[string]string{ ctlconf.ImagesLockKbldID: i.UnprocessedImageURL.URL, } - imageMetas := i.Metas - if len(imageMetas) > 0 { - metaYaml, err := yaml.Marshal(imageMetas) + if len(i.Metas) > 0 { + bs, err := yaml.Marshal(i.Metas) if err != nil { return anns } - anns[ctlconf.ImagesLockKbldMetas] = string(metaYaml) + anns[ctlconf.ImagesLockKbldMetas] = string(bs) } return anns diff --git a/pkg/kbld/config/config.go b/pkg/kbld/config/config.go index 377a614b..1fff03ef 100644 --- a/pkg/kbld/config/config.go +++ b/pkg/kbld/config/config.go @@ -68,7 +68,7 @@ type ImageOverride struct { NewImage string `json:"newImage"` Preresolved bool `json:"preresolved,omitempty"` TagSelection *versions.VersionSelection `json:"tagSelection,omitempty"` - ImageMetas []Meta `json:",omitempty"` + ImageMetas []Meta `json:"metas,omitempty"` } type ImageDestination struct { @@ -169,7 +169,7 @@ func NewConfigFromImagesLock(res ctlres.Resource) (Config, error) { overridesConfig := NewConfig() for _, image := range imagesLock.Images { - imageMeta, err := metasHistory(image.Annotations[ImagesLockKbldMetas]) + imgMeta, err := NewMetasFromString(image.Annotations[ImagesLockKbldMetas]) if err != nil { return Config{}, fmt.Errorf("Unmarshaling %s as %s annotation: %s", res.Description(), ImagesLockKbldMetas, err) } @@ -179,7 +179,7 @@ func NewConfigFromImagesLock(res ctlres.Resource) (Config, error) { }, NewImage: image.Image, Preresolved: true, - ImageMetas: imageMeta, + ImageMetas: imgMeta, } overridesConfig.Overrides = append(overridesConfig.Overrides, iOverride) } diff --git a/pkg/kbld/config/image_meta.go b/pkg/kbld/config/image_meta.go index 2dc5c182..8181ad04 100644 --- a/pkg/kbld/config/image_meta.go +++ b/pkg/kbld/config/image_meta.go @@ -13,12 +13,20 @@ type Meta interface { meta() } -type ImageMeta struct { +type imageMeta struct { Metas []Meta } +const ( + GitMeta = "git" + LocalMeta = "local" + ResolvedMeta = "resolved" + TaggedMeta = "tagged" + PreresolvedMeta = "preresolved" +) + type BuiltImageSourceGit struct { - Type string // always set to 'git' + Type string // always set to GitMeta RemoteURL string `json:",omitempty" yaml:",omitempty"` SHA string Dirty bool @@ -26,23 +34,23 @@ type BuiltImageSourceGit struct { } type BuiltImageSourceLocal struct { - Type string // always set to 'local' + Type string // always set to LocalMeta Path string } type ResolvedImageSourceURL struct { - Type string // always set to 'resolved' + Type string // always set to ResolvedMeta URL string Tag string } type TaggedImageMeta struct { - Type string // always set to 'tagged' + Type string // always set to TaggedMeta Tags []string } type PreresolvedImageSourceURL struct { - Type string // always set to 'preresolved' + Type string // always set to PreresolvedMeta URL string Tag string `json:",omitempty" yaml:",omitempty"` } @@ -53,18 +61,18 @@ func (ResolvedImageSourceURL) meta() {} func (TaggedImageMeta) meta() {} func (PreresolvedImageSourceURL) meta() {} -func metasHistory(metas string) ([]Meta, error) { - imageMeta := ImageMeta{} - err := yaml.Unmarshal([]byte(metas), &imageMeta) +func NewMetasFromString(metas string) ([]Meta, error) { + imgMeta := imageMeta{} + err := yaml.Unmarshal([]byte(metas), &imgMeta) if err != nil { return []Meta{}, err } - return imageMeta.Metas, nil + return imgMeta.Metas, nil } -var _ json.Unmarshaler = &ImageMeta{} +var _ json.Unmarshaler = &imageMeta{} -func (m *ImageMeta) UnmarshalJSON(data []byte) error { +func (m *imageMeta) UnmarshalJSON(data []byte) error { var list []interface{} err := yaml.Unmarshal(data, &list) if err != nil { @@ -81,16 +89,31 @@ func (m *ImageMeta) UnmarshalJSON(data []byte) error { yamlItem, _ := yaml.Marshal(&item) switch { - case yaml.Unmarshal(yamlItem, &local) == nil && local.Type == "local": + case yaml.Unmarshal(yamlItem, &local) == nil && local.Type == LocalMeta: m.Metas = append(m.Metas, local) - case yaml.Unmarshal(yamlItem, &git) == nil && git.Type == "git": + case yaml.Unmarshal(yamlItem, &git) == nil && git.Type == GitMeta: m.Metas = append(m.Metas, git) - case yaml.Unmarshal(yamlItem, &res) == nil && res.Type == "resolved": + case yaml.Unmarshal(yamlItem, &res) == nil && res.Type == ResolvedMeta: m.Metas = append(m.Metas, res) - case yaml.Unmarshal(yamlItem, &preres) == nil && preres.Type == "preresolved": + case yaml.Unmarshal(yamlItem, &preres) == nil && preres.Type == PreresolvedMeta: m.Metas = append(m.Metas, preres) - case yaml.Unmarshal(yamlItem, &tag) == nil && tag.Type == "tagged": + case yaml.Unmarshal(yamlItem, &tag) == nil && tag.Type == TaggedMeta: m.Metas = append(m.Metas, tag) + default: + // ignore unknown meta. + // At this time... + // - "Meta" are provided as primarily optional diagnostic information + // rather than operational data (read: less important). Losing + // this information does not change the correctness of kbld's + // primary purpose during deployment: to rewrite image references. + // It would be more than an annoyance to error-out if we were + // unable to parse such data. + // - Ideally, yes, we'd at least report a warning. However, if there's + // a systemic condition (e.g. using an older version of kbld to + // deploy than was used to package) there would likely be a flurry + // of warnings. So, the feature would quickly need an enhancement + // to de-dup such warnings. (read: added complexity) + // see also https://github.com/vmware-tanzu/carvel-kbld/issues/160 } } return nil diff --git a/pkg/kbld/image/built.go b/pkg/kbld/image/built.go index c6648e23..3962f966 100644 --- a/pkg/kbld/image/built.go +++ b/pkg/kbld/image/built.go @@ -117,7 +117,7 @@ func (i BuiltImage) sources() ([]ctlconf.Meta, error) { } sources = append(sources, ctlconf.BuiltImageSourceLocal{ - Type: "local", + Type: ctlconf.LocalMeta, Path: absPath, }) @@ -125,7 +125,7 @@ func (i BuiltImage) sources() ([]ctlconf.Meta, error) { if gitRepo.IsValid() { var err error - git := ctlconf.BuiltImageSourceGit{Type: "git"} + git := ctlconf.BuiltImageSourceGit{Type: ctlconf.GitMeta} git.RemoteURL, err = gitRepo.RemoteURL() if err != nil { diff --git a/pkg/kbld/image/preresolved.go b/pkg/kbld/image/preresolved.go index 4242a096..7a263eeb 100644 --- a/pkg/kbld/image/preresolved.go +++ b/pkg/kbld/image/preresolved.go @@ -13,11 +13,16 @@ type PreresolvedImage struct { } func NewPreresolvedImage(url string, metas []ctlconf.Meta) PreresolvedImage { - return PreresolvedImage{url, metas} + return PreresolvedImage{url, copyAndAppendMeta(metas)} } func (i PreresolvedImage) URL() (string, []ctlconf.Meta, error) { - imageMetas := append(i.metas, ctlconf.PreresolvedImageSourceURL{Type: "preresolved", URL: i.url}) - + imageMetas := copyAndAppendMeta(i.metas, ctlconf.PreresolvedImageSourceURL{Type: ctlconf.PreresolvedMeta, URL: i.url}) return i.url, imageMetas, nil } + +func copyAndAppendMeta(existing []ctlconf.Meta, new ...ctlconf.Meta) []ctlconf.Meta { + all := make([]ctlconf.Meta, len(existing), len(existing)+len(new)) + copy(all, existing) + return append(all, new...) +} diff --git a/pkg/kbld/image/resolved.go b/pkg/kbld/image/resolved.go index d298a802..40c0abca 100644 --- a/pkg/kbld/image/resolved.go +++ b/pkg/kbld/image/resolved.go @@ -49,7 +49,7 @@ func (i ResolvedImage) URL() (string, []ctlconf.Meta, error) { return "", nil, err } - metas = append(metas, ctlconf.ResolvedImageSourceURL{Type: "resolved", URL: i.url, Tag: tag.TagStr()}) + metas = append(metas, ctlconf.ResolvedImageSourceURL{Type: ctlconf.ResolvedMeta, URL: i.url, Tag: tag.TagStr()}) return url, metas, nil } diff --git a/pkg/kbld/image/tagged.go b/pkg/kbld/image/tagged.go index 99740d2d..646dc7c6 100644 --- a/pkg/kbld/image/tagged.go +++ b/pkg/kbld/image/tagged.go @@ -46,7 +46,7 @@ func (i TaggedImage) URL() (string, []ctlconf.Meta, error) { } } - metas = append(metas, ctlconf.TaggedImageMeta{Type: "tagged", Tags: i.imgDst.Tags}) + metas = append(metas, ctlconf.TaggedImageMeta{Type: ctlconf.TaggedMeta, Tags: i.imgDst.Tags}) } return url, metas, err