From 960215e329fd33e40191d5edc87aa5c255b42d45 Mon Sep 17 00:00:00 2001 From: Jim Crossley Date: Wed, 11 Sep 2019 09:45:55 -0400 Subject: [PATCH] Uses a pre-release of manifestival to fix #138 This relies on the Transform function now being immutable, returning a new Manifest instead of mutating its source resources. This is related to #163 as well. --- Gopkg.lock | 6 ++--- Gopkg.toml | 2 +- .../knativeserving_controller.go | 26 ++++++++++--------- .../jcrossley3/manifestival/manifestival.go | 14 ++++++---- .../jcrossley3/manifestival/transform.go | 13 +++++----- 5 files changed, 34 insertions(+), 27 deletions(-) diff --git a/Gopkg.lock b/Gopkg.lock index 86392086..b6b74de3 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -334,12 +334,12 @@ version = "v0.3.7" [[projects]] - branch = "client-go" - digest = "1:928c48472d9006c89f70c696ea1f7cb829b38e410e0641ec3f434080c2a90896" + branch = "immutable-transform" + digest = "1:5a6609de998eec16a99dd85dff44eda0b432a1152488450451098059fdfdb8e6" name = "github.com/jcrossley3/manifestival" packages = ["."] pruneopts = "NUT" - revision = "97e164e00f2355c8d3816d9006802224ba613a60" + revision = "e5b28800a2bbc4342940c8717a770030905f5519" [[projects]] digest = "1:1f2aebae7e7c856562355ec0198d8ca2fa222fb05e5b1b66632a1fce39631885" diff --git a/Gopkg.toml b/Gopkg.toml index c657b29a..93e9337d 100644 --- a/Gopkg.toml +++ b/Gopkg.toml @@ -56,7 +56,7 @@ required = [ [[constraint]] name = "github.com/jcrossley3/manifestival" - branch = "client-go" + branch = "immutable-transform" [[constraint]] name = "knative.dev/pkg" diff --git a/pkg/reconciler/knativeserving/knativeserving_controller.go b/pkg/reconciler/knativeserving/knativeserving_controller.go index a51a68c8..29fe3d79 100644 --- a/pkg/reconciler/knativeserving/knativeserving_controller.go +++ b/pkg/reconciler/knativeserving/knativeserving_controller.go @@ -158,32 +158,30 @@ func (r *Reconciler) install(instance *servingv1alpha1.KnativeServing) error { r.Logger.Debug("Installing manifest") defer r.updateStatus(instance) - if err := r.transform(instance); err != nil { + transformed, err := r.transform(instance) + if err != nil { return err } - if err := r.apply(instance); err != nil { + if err := r.apply(transformed, instance); err != nil { return err } return nil } // Transform the resources -func (r *Reconciler) transform(instance *servingv1alpha1.KnativeServing) error { +func (r *Reconciler) transform(instance *servingv1alpha1.KnativeServing) (*mf.Manifest, error) { r.Logger.Debug("Transforming manifest") - transforms, err := platform.Transformers(r.KubeClientSet, instance, r.Logger) + transforms, err := platform.Transformers(r.KubeClientSet, instance) if err != nil { - return err - } - if err := r.config.Transform(transforms...); err != nil { - return err + return nil, err } - return nil + return r.config.Transform(transforms...) } // Apply the embedded resources -func (r *Reconciler) apply(instance *servingv1alpha1.KnativeServing) error { +func (r *Reconciler) apply(manifest mf.Manifestival, instance *servingv1alpha1.KnativeServing) error { r.Logger.Debug("Applying manifest") - if err := r.config.ApplyAll(); err != nil { + if err := manifest.ApplyAll(); err != nil { instance.Status.MarkInstallFailed(err.Error()) return err } @@ -204,7 +202,11 @@ func (r *Reconciler) checkDeployments(instance *servingv1alpha1.KnativeServing) } return false } - for _, u := range r.config.Resources { + transformed, err := r.transform(instance) + if err != nil { + return err + } + for _, u := range transformed.Resources { if u.GetKind() == "Deployment" { deployment, err := r.KubeClientSet.AppsV1().Deployments(u.GetNamespace()).Get(u.GetName(), metav1.GetOptions{}) if err != nil { diff --git a/vendor/github.com/jcrossley3/manifestival/manifestival.go b/vendor/github.com/jcrossley3/manifestival/manifestival.go index 6445f397..ded162ef 100644 --- a/vendor/github.com/jcrossley3/manifestival/manifestival.go +++ b/vendor/github.com/jcrossley3/manifestival/manifestival.go @@ -34,7 +34,7 @@ type Manifestival interface { // Returns a copy of the resource from the api server, nil if not found Get(spec *unstructured.Unstructured) (*unstructured.Unstructured, error) // Transforms the resources within a Manifest - Transform(fns ...Transformer) error + Transform(fns ...Transformer) (*Manifest, error) } type Manifest struct { @@ -161,14 +161,18 @@ func (f *Manifest) ResourceInterface(spec *unstructured.Unstructured) (dynamic.R return f.client.Resource(mapping.Resource).Namespace(spec.GetNamespace()), nil } -// We need to preserve the top-level target keys, specifically -// 'metadata.resourceVersion', 'spec.clusterIP', and any existing -// entries in a ConfigMap's 'data' field. So we only overwrite fields -// set in our src resource. +// We need to preserve some target keys, specifically +// 'metadata.resourceVersion' and 'spec.clusterIP'. So we only +// overwrite fields set in our src resource. // TODO: Use Patch instead func UpdateChanged(src, tgt map[string]interface{}) bool { changed := false for k, v := range src { + // Special case for ConfigMaps + if k == "data" && !equality.Semantic.DeepEqual(v, tgt[k]) { + tgt[k], changed = v, true + continue + } if v, ok := v.(map[string]interface{}); ok { if tgt[k] == nil { tgt[k], changed = v, true diff --git a/vendor/github.com/jcrossley3/manifestival/transform.go b/vendor/github.com/jcrossley3/manifestival/transform.go index e43f5bd8..4480de44 100644 --- a/vendor/github.com/jcrossley3/manifestival/transform.go +++ b/vendor/github.com/jcrossley3/manifestival/transform.go @@ -18,20 +18,21 @@ type Owner interface { } // If an error occurs, no resources are transformed -func (f *Manifest) Transform(fns ...Transformer) error { +func (f *Manifest) Transform(fns ...Transformer) (*Manifest, error) { var results []unstructured.Unstructured for i := 0; i < len(f.Resources); i++ { spec := f.Resources[i].DeepCopy() for _, transform := range fns { - err := transform(spec) - if err != nil { - return err + if transform != nil { + err := transform(spec) + if err != nil { + return nil, err + } } } results = append(results, *spec) } - f.Resources = results - return nil + return &Manifest{Resources: results, client: f.client, mapper: f.mapper}, nil } // We assume all resources in the manifest live in the same namespace