Skip to content

Commit

Permalink
fix(service update): Only update fields which have been sent by serve…
Browse files Browse the repository at this point in the history 
…r. (#155)

* fix(service update): Only update fields which have been sent by server.

This reflects the lemonade process step1. Tests have been adapted to
verify this behaviours.

The only situation when we update field coming from the server is for
"kn service update" for envs, image and requests/limits.

All other operation are either create (here, we always have to send the
old fields), or read (get/describe).

Fixes #144.

* chore: typo fix

* refactor(service update/create): Moved from Configuration to RevisionTemplateSpec

In order to proper handling the v1alpha1 -> v1beta1 migration methods has been updated to get rid fo Configuration within the service as this
is completely inlined in v1beta1.

The helper methods have been also updated accordingly.
I think we are good now.
  • Loading branch information
@rhuss @knative-prow-robot
rhuss authored and knative-prow-robot committed Jun 3, 2019
1 parent 43061af commit b6a8fa9
Show file tree
Hide file tree
Showing 8 changed files with 261 additions and 140 deletions.
16 changes: 11 additions & 5 deletions pkg/kn/commands/configuration_edit_flags.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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
}
Expand All @@ -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
}
Expand Down
7 changes: 1 addition & 6 deletions pkg/kn/commands/service_create.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
}
Expand Down
74 changes: 37 additions & 37 deletions pkg/kn/commands/service_create_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
}
}

Expand All @@ -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)
}
}

Expand All @@ -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)
}
}

Expand All @@ -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)
}
}
}
Expand All @@ -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)
}
}
}
Expand All @@ -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)
}
}
}
Expand All @@ -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)
}
Expand All @@ -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)
}
Expand Down
8 changes: 2 additions & 6 deletions pkg/kn/commands/service_update.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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
}
Expand Down
Loading

0 comments on commit b6a8fa9

Please sign in to comment.