diff --git a/pkg/kn/commands/configuration_edit_flags.go b/pkg/kn/commands/configuration_edit_flags.go index 8f24e158f..184e04684 100644 --- a/pkg/kn/commands/configuration_edit_flags.go +++ b/pkg/kn/commands/configuration_edit_flags.go @@ -54,7 +54,13 @@ func (p *ConfigurationEditFlags) AddCreateFlags(command *cobra.Command) { command.MarkFlagRequired("image") } -func (p *ConfigurationEditFlags) Apply(config *servingv1alpha1.ConfigurationSpec, cmd *cobra.Command) error { +func (p *ConfigurationEditFlags) Apply(service *servingv1alpha1.Service, cmd *cobra.Command) error { + + template, err := servinglib.GetRevisionTemplate(service) + if err != nil { + return err + } + envMap := map[string]string{} for _, pairStr := range p.Env { pairSlice := strings.SplitN(pairStr, "=", 2) @@ -65,12 +71,12 @@ func (p *ConfigurationEditFlags) Apply(config *servingv1alpha1.ConfigurationSpec } envMap[pairSlice[0]] = pairSlice[1] } - err := servinglib.UpdateEnvVars(config, envMap) - if err != nil { + if err := servinglib.UpdateEnvVars(template, envMap); err != nil { return err } + if cmd.Flags().Changed("image") { - err = servinglib.UpdateImage(config, p.Image) + err = servinglib.UpdateImage(template, p.Image) if err != nil { return err } @@ -83,7 +89,7 @@ func (p *ConfigurationEditFlags) Apply(config *servingv1alpha1.ConfigurationSpec if err != nil { return err } - err = servinglib.UpdateResources(config, requestsResources, limitsResources) + err = servinglib.UpdateResources(template, requestsResources, limitsResources) if err != nil { return err } diff --git a/pkg/kn/commands/service_create.go b/pkg/kn/commands/service_create.go index 50a81bb39..dcda99095 100644 --- a/pkg/kn/commands/service_create.go +++ b/pkg/kn/commands/service_create.go @@ -18,7 +18,6 @@ import ( "errors" "fmt" - serving_lib "github.com/knative/client/pkg/serving" servingv1alpha1 "github.com/knative/serving/pkg/apis/serving/v1alpha1" "github.com/spf13/cobra" corev1 "k8s.io/api/core/v1" @@ -81,11 +80,7 @@ func NewServiceCreateCommand(p *KnParams) *cobra.Command { }, } - config, err := serving_lib.GetConfiguration(&service) - if err != nil { - return err - } - err = editFlags.Apply(config, cmd) + err = editFlags.Apply(&service, cmd) if err != nil { return err } diff --git a/pkg/kn/commands/service_create_test.go b/pkg/kn/commands/service_create_test.go index c08e1a7ac..7cfc014dd 100644 --- a/pkg/kn/commands/service_create_test.go +++ b/pkg/kn/commands/service_create_test.go @@ -74,11 +74,11 @@ func TestServiceCreateImage(t *testing.T) { } else if !action.Matches("create", "services") { t.Fatalf("Bad action %v", action) } - conf, err := servinglib.GetConfiguration(created) + template, err := servinglib.GetRevisionTemplate(created) if err != nil { t.Fatal(err) - } else if conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Image != "gcr.io/foo/bar:baz" { - t.Fatalf("wrong image set: %v", conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Image) + } else if template.Spec.DeprecatedContainer.Image != "gcr.io/foo/bar:baz" { + t.Fatalf("wrong image set: %v", template.Spec.DeprecatedContainer.Image) } else if !strings.Contains(output, "foo") || !strings.Contains(output, "created") || !strings.Contains(output, "default") { t.Fatalf("wrong stdout message: %v", output) @@ -99,20 +99,20 @@ func TestServiceCreateEnv(t *testing.T) { "A": "DOGS", "B": "WOLVES"} - conf, err := servinglib.GetConfiguration(created) - actualEnvVars, err := servinglib.EnvToMap(conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Env) + template, err := servinglib.GetRevisionTemplate(created) + actualEnvVars, err := servinglib.EnvToMap(template.Spec.DeprecatedContainer.Env) if err != nil { t.Fatal(err) } if err != nil { t.Fatal(err) - } else if conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Image != "gcr.io/foo/bar:baz" { - t.Fatalf("wrong image set: %v", conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Image) + } else if template.Spec.DeprecatedContainer.Image != "gcr.io/foo/bar:baz" { + t.Fatalf("wrong image set: %v", template.Spec.DeprecatedContainer.Image) } else if !reflect.DeepEqual( actualEnvVars, expectedEnvVars) { - t.Fatalf("wrong env vars %v", conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Env) + t.Fatalf("wrong env vars %v", template.Spec.DeprecatedContainer.Env) } } @@ -131,14 +131,14 @@ func TestServiceCreateWithRequests(t *testing.T) { corev1.ResourceMemory: parseQuantity(t, "64Mi"), } - conf, err := servinglib.GetConfiguration(created) + template, err := servinglib.GetRevisionTemplate(created) if err != nil { t.Fatal(err) } else if !reflect.DeepEqual( - conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Requests, + template.Spec.DeprecatedContainer.Resources.Requests, expectedRequestsVars) { - t.Fatalf("wrong requests vars %v", conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Requests) + t.Fatalf("wrong requests vars %v", template.Spec.DeprecatedContainer.Resources.Requests) } } @@ -157,14 +157,14 @@ func TestServiceCreateWithLimits(t *testing.T) { corev1.ResourceMemory: parseQuantity(t, "1024Mi"), } - conf, err := servinglib.GetConfiguration(created) + template, err := servinglib.GetRevisionTemplate(created) if err != nil { t.Fatal(err) } else if !reflect.DeepEqual( - conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Limits, + template.Spec.DeprecatedContainer.Resources.Limits, expectedLimitsVars) { - t.Fatalf("wrong limits vars %v", conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Limits) + t.Fatalf("wrong limits vars %v", template.Spec.DeprecatedContainer.Resources.Limits) } } @@ -186,21 +186,21 @@ func TestServiceCreateRequestsLimitsCPU(t *testing.T) { corev1.ResourceCPU: parseQuantity(t, "1000m"), } - conf, err := servinglib.GetConfiguration(created) + template, err := servinglib.GetRevisionTemplate(created) if err != nil { t.Fatal(err) } else { if !reflect.DeepEqual( - conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Requests, + template.Spec.DeprecatedContainer.Resources.Requests, expectedRequestsVars) { - t.Fatalf("wrong requests vars %v", conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Requests) + t.Fatalf("wrong requests vars %v", template.Spec.DeprecatedContainer.Resources.Requests) } if !reflect.DeepEqual( - conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Limits, + template.Spec.DeprecatedContainer.Resources.Limits, expectedLimitsVars) { - t.Fatalf("wrong limits vars %v", conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Limits) + t.Fatalf("wrong limits vars %v", template.Spec.DeprecatedContainer.Resources.Limits) } } } @@ -223,21 +223,21 @@ func TestServiceCreateRequestsLimitsMemory(t *testing.T) { corev1.ResourceMemory: parseQuantity(t, "1024Mi"), } - conf, err := servinglib.GetConfiguration(created) + template, err := servinglib.GetRevisionTemplate(created) if err != nil { t.Fatal(err) } else { if !reflect.DeepEqual( - conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Requests, + template.Spec.DeprecatedContainer.Resources.Requests, expectedRequestsVars) { - t.Fatalf("wrong requests vars %v", conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Requests) + t.Fatalf("wrong requests vars %v", template.Spec.DeprecatedContainer.Resources.Requests) } if !reflect.DeepEqual( - conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Limits, + template.Spec.DeprecatedContainer.Resources.Limits, expectedLimitsVars) { - t.Fatalf("wrong limits vars %v", conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Limits) + t.Fatalf("wrong limits vars %v", template.Spec.DeprecatedContainer.Resources.Limits) } } } @@ -264,21 +264,21 @@ func TestServiceCreateRequestsLimitsCPUMemory(t *testing.T) { corev1.ResourceMemory: parseQuantity(t, "1024Mi"), } - conf, err := servinglib.GetConfiguration(created) + template, err := servinglib.GetRevisionTemplate(created) if err != nil { t.Fatal(err) } else { if !reflect.DeepEqual( - conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Requests, + template.Spec.DeprecatedContainer.Resources.Requests, expectedRequestsVars) { - t.Fatalf("wrong requests vars %v", conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Requests) + t.Fatalf("wrong requests vars %v", template.Spec.DeprecatedContainer.Resources.Requests) } if !reflect.DeepEqual( - conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Limits, + template.Spec.DeprecatedContainer.Resources.Limits, expectedLimitsVars) { - t.Fatalf("wrong limits vars %v", conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Limits) + t.Fatalf("wrong limits vars %v", template.Spec.DeprecatedContainer.Resources.Limits) } } } @@ -304,11 +304,11 @@ func TestServiceCreateImageForce(t *testing.T) { } else if !action.Matches("create", "services") { t.Fatalf("Bad action %v", action) } - conf, err := servinglib.GetConfiguration(created) + template, err := servinglib.GetRevisionTemplate(created) if err != nil { t.Fatal(err) - } else if conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Image != "gcr.io/foo/bar:v2" { - t.Fatalf("wrong image set: %v", conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Image) + } else if template.Spec.DeprecatedContainer.Image != "gcr.io/foo/bar:v2" { + t.Fatalf("wrong image set: %v", template.Spec.DeprecatedContainer.Image) } else if !strings.Contains(output, "foo") || !strings.Contains(output, "default") { t.Fatalf("wrong output: %s", output) } @@ -333,19 +333,19 @@ func TestServiceCreateEnvForce(t *testing.T) { "A": "CATS", "B": "LIONS"} - conf, err := servinglib.GetConfiguration(created) - actualEnvVars, err := servinglib.EnvToMap(conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Env) + template, err := servinglib.GetRevisionTemplate(created) + actualEnvVars, err := servinglib.EnvToMap(template.Spec.DeprecatedContainer.Env) if err != nil { t.Fatal(err) } if err != nil { t.Fatal(err) - } else if conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Image != "gcr.io/foo/bar:v2" { - t.Fatalf("wrong image set: %v", conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Image) + } else if template.Spec.DeprecatedContainer.Image != "gcr.io/foo/bar:v2" { + t.Fatalf("wrong image set: %v", template.Spec.DeprecatedContainer.Image) } else if !reflect.DeepEqual( actualEnvVars, expectedEnvVars) { - t.Fatalf("wrong env vars:%v", conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Env) + t.Fatalf("wrong env vars:%v", template.Spec.DeprecatedContainer.Env) } else if !strings.Contains(output, "foo") || !strings.Contains(output, "default") { t.Fatalf("wrong output: %s", output) } diff --git a/pkg/kn/commands/service_update.go b/pkg/kn/commands/service_update.go index 0d80e5b9a..1fad551b8 100644 --- a/pkg/kn/commands/service_update.go +++ b/pkg/kn/commands/service_update.go @@ -17,7 +17,6 @@ package commands import ( "errors" - serving_lib "github.com/knative/client/pkg/serving" "github.com/spf13/cobra" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -53,12 +52,9 @@ func NewServiceUpdateCommand(p *KnParams) *cobra.Command { if err != nil { return err } + service = service.DeepCopy() - config, err := serving_lib.GetConfiguration(service) - if err != nil { - return err - } - err = editFlags.Apply(config, cmd) + err = editFlags.Apply(service, cmd) if err != nil { return err } diff --git a/pkg/kn/commands/service_update_test.go b/pkg/kn/commands/service_update_test.go index 6e3869fc0..6bbe450ed 100644 --- a/pkg/kn/commands/service_update_test.go +++ b/pkg/kn/commands/service_update_test.go @@ -93,12 +93,12 @@ func TestServiceUpdateImage(t *testing.T) { }, } - config, err := servinglib.GetConfiguration(orig) + template, err := servinglib.GetRevisionTemplate(orig) if err != nil { t.Fatal(err) } - servinglib.UpdateImage(config, "gcr.io/foo/bar:baz") + servinglib.UpdateImage(template, "gcr.io/foo/bar:baz") action, updated, _, err := fakeServiceUpdate(orig, []string{ "service", "update", "foo", "--image", "gcr.io/foo/quux:xyzzy"}) @@ -108,11 +108,12 @@ func TestServiceUpdateImage(t *testing.T) { } else if !action.Matches("update", "services") { t.Fatalf("Bad action %v", action) } - conf, err := servinglib.GetConfiguration(updated) + + template, err = servinglib.GetRevisionTemplate(updated) if err != nil { t.Fatal(err) - } else if conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Image != "gcr.io/foo/quux:xyzzy" { - t.Fatalf("wrong image set: %v", conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Image) + } else if template.Spec.DeprecatedContainer.Image != "gcr.io/foo/quux:xyzzy" { + t.Fatalf("wrong image set: %v", template.Spec.DeprecatedContainer.Image) } } @@ -139,12 +140,12 @@ func TestServiceUpdateEnv(t *testing.T) { }, } - config, err := servinglib.GetConfiguration(orig) + template, err := servinglib.GetRevisionTemplate(orig) if err != nil { t.Fatal(err) } - servinglib.UpdateImage(config, "gcr.io/foo/bar:baz") + servinglib.UpdateImage(template, "gcr.io/foo/bar:baz") action, updated, _, err := fakeServiceUpdate(orig, []string{ "service", "update", "foo", "-e", "TARGET=Awesome"}) @@ -159,13 +160,13 @@ func TestServiceUpdateEnv(t *testing.T) { Value: "Awesome", } - conf, err := servinglib.GetConfiguration(updated) + template, err = servinglib.GetRevisionTemplate(updated) if err != nil { t.Fatal(err) - } else if conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Image != "gcr.io/foo/bar:baz" { - t.Fatalf("wrong image set: %v", conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Image) - } else if conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Env[0] != expectedEnvVar { - t.Fatalf("wrong env set: %v", conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Env) + } else if template.Spec.DeprecatedContainer.Image != "gcr.io/foo/bar:baz" { + t.Fatalf("wrong image set: %v", template.Spec.DeprecatedContainer.Image) + } else if template.Spec.DeprecatedContainer.Env[0] != expectedEnvVar { + t.Fatalf("wrong env set: %v", template.Spec.DeprecatedContainer.Env) } } @@ -189,20 +190,20 @@ func TestServiceUpdateRequestsLimitsCPU(t *testing.T) { corev1.ResourceMemory: resource.MustParse("1024Mi"), } - newConfig, err := servinglib.GetConfiguration(updated) + newTemplate, err := servinglib.GetRevisionTemplate(updated) if err != nil { t.Fatal(err) } else { if !reflect.DeepEqual( - newConfig.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Requests, + newTemplate.Spec.DeprecatedContainer.Resources.Requests, expectedRequestsVars) { - t.Fatalf("wrong requests vars %v", newConfig.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Requests) + t.Fatalf("wrong requests vars %v", newTemplate.Spec.DeprecatedContainer.Resources.Requests) } if !reflect.DeepEqual( - newConfig.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Limits, + newTemplate.Spec.DeprecatedContainer.Resources.Limits, expectedLimitsVars) { - t.Fatalf("wrong limits vars %v", newConfig.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Limits) + t.Fatalf("wrong limits vars %v", newTemplate.Spec.DeprecatedContainer.Resources.Limits) } } } @@ -227,20 +228,20 @@ func TestServiceUpdateRequestsLimitsMemory(t *testing.T) { corev1.ResourceMemory: resource.MustParse("2048Mi"), } - newConfig, err := servinglib.GetConfiguration(updated) + newTemplate, err := servinglib.GetRevisionTemplate(updated) if err != nil { t.Fatal(err) } else { if !reflect.DeepEqual( - newConfig.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Requests, + newTemplate.Spec.DeprecatedContainer.Resources.Requests, expectedRequestsVars) { - t.Fatalf("wrong requests vars %v", newConfig.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Requests) + t.Fatalf("wrong requests vars %v", newTemplate.Spec.DeprecatedContainer.Resources.Requests) } if !reflect.DeepEqual( - newConfig.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Limits, + newTemplate.Spec.DeprecatedContainer.Resources.Limits, expectedLimitsVars) { - t.Fatalf("wrong limits vars %v", newConfig.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Limits) + t.Fatalf("wrong limits vars %v", newTemplate.Spec.DeprecatedContainer.Resources.Limits) } } } @@ -267,20 +268,20 @@ func TestServiceUpdateRequestsLimitsCPU_and_Memory(t *testing.T) { corev1.ResourceMemory: resource.MustParse("2048Mi"), } - newConfig, err := servinglib.GetConfiguration(updated) + newTemplate, err := servinglib.GetRevisionTemplate(updated) if err != nil { t.Fatal(err) } else { if !reflect.DeepEqual( - newConfig.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Requests, + newTemplate.Spec.DeprecatedContainer.Resources.Requests, expectedRequestsVars) { - t.Fatalf("wrong requests vars %v", newConfig.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Requests) + t.Fatalf("wrong requests vars %v", newTemplate.Spec.DeprecatedContainer.Resources.Requests) } if !reflect.DeepEqual( - newConfig.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Limits, + newTemplate.Spec.DeprecatedContainer.Resources.Limits, expectedLimitsVars) { - t.Fatalf("wrong limits vars %v", newConfig.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Limits) + t.Fatalf("wrong limits vars %v", newTemplate.Spec.DeprecatedContainer.Resources.Limits) } } } @@ -308,12 +309,12 @@ func createMockServiceWithResources(t *testing.T, requestCPU, requestMemory, lim }, } - config, err := servinglib.GetConfiguration(service) + template, err := servinglib.GetRevisionTemplate(service) if err != nil { t.Fatal(err) } - config.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources = corev1.ResourceRequirements{ + template.Spec.DeprecatedContainer.Resources = corev1.ResourceRequirements{ Requests: corev1.ResourceList{ corev1.ResourceCPU: resource.MustParse(requestCPU), corev1.ResourceMemory: resource.MustParse(requestMemory), diff --git a/pkg/serving/config_changes.go b/pkg/serving/config_changes.go index 56430cc79..f9ee71bb3 100644 --- a/pkg/serving/config_changes.go +++ b/pkg/serving/config_changes.go @@ -24,28 +24,13 @@ import ( // Give the configuration all the env var values listed in the given map of // vars. Does not touch any environment variables not mentioned, but it can add // new env vars and change the values of existing ones. -func UpdateEnvVars(config *servingv1alpha1.ConfigurationSpec, vars map[string]string) error { - set := make(map[string]bool) - for i, _ := range config.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Env { - envVar := &config.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Env[i] - value, present := vars[envVar.Name] - if present { - envVar.Value = value - set[envVar.Name] = true - } - } - for name, value := range vars { - if !set[name] { - config.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Env = append( - config.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Env, - corev1.EnvVar{ - Name: name, - Value: value, - }) - } +func UpdateEnvVars(template *servingv1alpha1.RevisionTemplateSpec, vars map[string]string) error { + container, err := extractContainer(template) + if err != nil { + return err } + container.Env = updateEnvVarsFromMap(container.Env, vars) return nil - } // Utility function to translate between the API list form of env vars, and the @@ -55,34 +40,88 @@ func EnvToMap(vars []corev1.EnvVar) (map[string]string, error) { for _, envVar := range vars { _, present := result[envVar.Name] if present { - return nil, fmt.Errorf("Env var name present more than once: %v", envVar.Name) + return nil, fmt.Errorf("env var name present more than once: %v", envVar.Name) } result[envVar.Name] = envVar.Value } return result, nil } -func UpdateImage(config *servingv1alpha1.ConfigurationSpec, image string) error { - config.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Image = image +// Update a given image +func UpdateImage(template *servingv1alpha1.RevisionTemplateSpec, image string) error { + container, err := extractContainer(template) + if err != nil { + return err + } + container.Image = image return nil } -func UpdateResources(config *servingv1alpha1.ConfigurationSpec, requestsResourceList corev1.ResourceList, limitsResourceList corev1.ResourceList) error { - if config.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Requests == nil { - config.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Requests = corev1.ResourceList{} +func UpdateResources(template *servingv1alpha1.RevisionTemplateSpec, requestsResourceList corev1.ResourceList, limitsResourceList corev1.ResourceList) error { + container, err := extractContainer(template) + if err != nil { + return err + } + if container.Resources.Requests == nil { + container.Resources.Requests = corev1.ResourceList{} } for k, v := range requestsResourceList { - config.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Requests[k] = v + container.Resources.Requests[k] = v } - if config.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Limits == nil { - config.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Limits = corev1.ResourceList{} + if container.Resources.Limits == nil { + container.Resources.Limits = corev1.ResourceList{} } for k, v := range limitsResourceList { - config.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Limits[k] = v + container.Resources.Limits[k] = v } return nil } + +// ======================================================================================= + +func usesOldV1alpha1ContainerField(revision *servingv1alpha1.RevisionTemplateSpec) bool { + return revision.Spec.DeprecatedContainer != nil +} + +func extractContainer(template *servingv1alpha1.RevisionTemplateSpec) (*corev1.Container, error) { + if usesOldV1alpha1ContainerField(template) { + return template.Spec.DeprecatedContainer, nil + } + containers := template.Spec.Containers + if len(containers) == 0 { + return nil, fmt.Errorf("internal: no container set in spec.template.spec.containers") + } + if len(containers) > 1 { + return nil, fmt.Errorf("internal: can't extract container for updating environment"+ + " variables as the configuration contains "+ + "more than one container (i.e. %d containers)", len(containers)) + } + return &containers[0], nil +} + +func updateEnvVarsFromMap(env []corev1.EnvVar, vars map[string]string) []corev1.EnvVar { + set := make(map[string]bool) + for i, _ := range env { + envVar := &env[i] + value, present := vars[envVar.Name] + if present { + envVar.Value = value + set[envVar.Name] = true + } + } + for name, value := range vars { + if !set[name] { + env = append( + env, + corev1.EnvVar{ + Name: name, + Value: value, + }) + } + } + return env +} diff --git a/pkg/serving/config_changes_test.go b/pkg/serving/config_changes_test.go index d4dbe2d0b..35c8d4c13 100644 --- a/pkg/serving/config_changes_test.go +++ b/pkg/serving/config_changes_test.go @@ -15,6 +15,7 @@ package serving import ( + "github.com/knative/serving/pkg/apis/serving/v1beta1" "reflect" "testing" @@ -23,16 +24,25 @@ import ( ) func TestUpdateEnvVarsNew(t *testing.T) { - config := getEmptyConfigurationSpec() + template, container := getV1alpha1RevisionTemplateWithOldFields() + testUpdateEnvVarsNew(t, template, container) + assertNoV1alpha1(t, template) + + template, container = getV1alpha1Config() + testUpdateEnvVarsNew(t, template, container) + assertNoV1alpha1Old(t, template) +} + +func testUpdateEnvVarsNew(t *testing.T, template *servingv1alpha1.RevisionTemplateSpec, container *corev1.Container) { env := map[string]string{ "a": "foo", "b": "bar", } - err := UpdateEnvVars(&config, env) + err := UpdateEnvVars(template, env) if err != nil { t.Fatal(err) } - found, err := EnvToMap(config.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Env) + found, err := EnvToMap(container.Env) if err != nil { t.Fatal(err) } @@ -41,14 +51,24 @@ func TestUpdateEnvVarsNew(t *testing.T) { } } -func TestUpdateEnvVarsAppend(t *testing.T) { - config := getEmptyConfigurationSpec() - config.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Env = []corev1.EnvVar{ - corev1.EnvVar{Name: "a", Value: "foo"}} +func TestUpdateEnvVarsAppendOld(t *testing.T) { + template, container := getV1alpha1RevisionTemplateWithOldFields() + testUpdateEnvVarsAppendOld(t, template, container) + assertNoV1alpha1(t, template) + + template, container = getV1alpha1Config() + testUpdateEnvVarsAppendOld(t, template, container) + assertNoV1alpha1Old(t, template) +} + +func testUpdateEnvVarsAppendOld(t *testing.T, template *servingv1alpha1.RevisionTemplateSpec, container *corev1.Container) { + container.Env = []corev1.EnvVar{ + {Name: "a", Value: "foo"}, + } env := map[string]string{ "b": "bar", } - err := UpdateEnvVars(&config, env) + err := UpdateEnvVars(template, env) if err != nil { t.Fatal(err) } @@ -58,23 +78,32 @@ func TestUpdateEnvVarsAppend(t *testing.T) { "b": "bar", } - found, err := EnvToMap(config.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Env) + found, err := EnvToMap(container.Env) if err != nil { t.Fatal(err) } if !reflect.DeepEqual(expected, found) { - t.Fatalf("Env did not match expected %v found %v", env, found) + t.Fatalf("Env did not match expected %v, found %v", env, found) } } func TestUpdateEnvVarsModify(t *testing.T) { - config := getEmptyConfigurationSpec() - config.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Env = []corev1.EnvVar{ + template, container := getV1alpha1RevisionTemplateWithOldFields() + testUpdateEnvVarsModify(t, template, container) + assertNoV1alpha1(t, template) + + template, container = getV1alpha1Config() + testUpdateEnvVarsModify(t, template, container) + assertNoV1alpha1Old(t, template) +} + +func testUpdateEnvVarsModify(t *testing.T, revision *servingv1alpha1.RevisionTemplateSpec, container *corev1.Container) { + container.Env = []corev1.EnvVar{ corev1.EnvVar{Name: "a", Value: "foo"}} env := map[string]string{ "a": "fancy", } - err := UpdateEnvVars(&config, env) + err := UpdateEnvVars(revision, env) if err != nil { t.Fatal(err) } @@ -83,25 +112,34 @@ func TestUpdateEnvVarsModify(t *testing.T) { "a": "fancy", } - found, err := EnvToMap(config.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Env) + found, err := EnvToMap(container.Env) if err != nil { t.Fatal(err) } if !reflect.DeepEqual(expected, found) { - t.Fatalf("Env did not match expected %v found %v", env, found) + t.Fatalf("Env did not match expected %v, found %v", env, found) } } func TestUpdateEnvVarsBoth(t *testing.T) { - config := getEmptyConfigurationSpec() - config.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Env = []corev1.EnvVar{ + template, container := getV1alpha1RevisionTemplateWithOldFields() + testUpdateEnvVarsBoth(t, template, container) + assertNoV1alpha1(t, template) + + template, container = getV1alpha1Config() + testUpdateEnvVarsBoth(t, template, container) + assertNoV1alpha1Old(t, template) +} + +func testUpdateEnvVarsBoth(t *testing.T, template *servingv1alpha1.RevisionTemplateSpec, container *corev1.Container) { + container.Env = []corev1.EnvVar{ corev1.EnvVar{Name: "a", Value: "foo"}, corev1.EnvVar{Name: "c", Value: "caroline"}} env := map[string]string{ "a": "fancy", "b": "boo", } - err := UpdateEnvVars(&config, env) + err := UpdateEnvVars(template, env) if err != nil { t.Fatal(err) } @@ -112,21 +150,49 @@ func TestUpdateEnvVarsBoth(t *testing.T) { "c": "caroline", } - found, err := EnvToMap(config.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Env) + found, err := EnvToMap(container.Env) if err != nil { t.Fatal(err) } if !reflect.DeepEqual(expected, found) { - t.Fatalf("Env did not match expected %v found %v", env, found) + t.Fatalf("Env did not match expected %v, found %v", env, found) } } -func getEmptyConfigurationSpec() servingv1alpha1.ConfigurationSpec { - return servingv1alpha1.ConfigurationSpec{ - DeprecatedRevisionTemplate: &servingv1alpha1.RevisionTemplateSpec{ - Spec: servingv1alpha1.RevisionSpec{ - DeprecatedContainer: &corev1.Container{}, +// ========================================================================================================= + +func getV1alpha1RevisionTemplateWithOldFields() (*servingv1alpha1.RevisionTemplateSpec, *corev1.Container) { + container := &corev1.Container{} + template := &servingv1alpha1.RevisionTemplateSpec{ + Spec: servingv1alpha1.RevisionSpec{ + DeprecatedContainer: container, + }, + } + return template, container +} + +func getV1alpha1Config() (*servingv1alpha1.RevisionTemplateSpec, *corev1.Container) { + containers := []corev1.Container{{}} + template := &servingv1alpha1.RevisionTemplateSpec{ + Spec: servingv1alpha1.RevisionSpec{ + RevisionSpec: v1beta1.RevisionSpec{ + PodSpec: v1beta1.PodSpec{ + Containers: containers, + }, }, }, } + return template, &containers[0] +} + +func assertNoV1alpha1Old(t *testing.T, template *servingv1alpha1.RevisionTemplateSpec) { + if template.Spec.DeprecatedContainer != nil { + t.Error("Assuming only new v1alpha1 fields but found spec.container") + } +} + +func assertNoV1alpha1(t *testing.T, template *servingv1alpha1.RevisionTemplateSpec) { + if template.Spec.Containers != nil { + t.Error("Assuming only old v1alpha1 fields but found spec.template") + } } diff --git a/pkg/serving/service.go b/pkg/serving/service.go index f146956a7..22a3b2c6e 100644 --- a/pkg/serving/service.go +++ b/pkg/serving/service.go @@ -20,7 +20,25 @@ import ( servingv1alpha1 "github.com/knative/serving/pkg/apis/serving/v1alpha1" ) -func GetConfiguration(service *servingv1alpha1.Service) (*servingv1alpha1.ConfigurationSpec, error) { +// Get the revision template associated with a service. +// Depending on the structure returned either the new v1beta1 fields or the +// 'old' v1alpha1 fields are looked up. +// The returned revision template can be updated in place. +// An error is returned if no revision template could be extracted +func GetRevisionTemplate(service *servingv1alpha1.Service) (*servingv1alpha1.RevisionTemplateSpec, error) { + // Try v1beta1 field first + if service.Spec.Template != nil { + return service.Spec.Template, nil + } + + config, err := getConfiguration(service) + if err != nil { + return nil, err + } + return config.DeprecatedRevisionTemplate, nil +} + +func getConfiguration(service *servingv1alpha1.Service) (*servingv1alpha1.ConfigurationSpec, error) { if service.Spec.DeprecatedRunLatest != nil { return &service.Spec.DeprecatedRunLatest.Configuration, nil } else if service.Spec.DeprecatedRelease != nil { @@ -28,6 +46,6 @@ func GetConfiguration(service *servingv1alpha1.Service) (*servingv1alpha1.Config } else if service.Spec.DeprecatedPinned != nil { return &service.Spec.DeprecatedPinned.Configuration, nil } else { - return nil, errors.New("Service does not specify a Configuration") + return nil, errors.New("service does not specify a Configuration") } }