-
Notifications
You must be signed in to change notification settings - Fork 263
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.
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
Add range options to --scale #1070
Changes from 3 commits
3db3cdb
7f91820
2edf4c4
84eb376
abb75e4
2b7631a
33d9193
efa9e27
537973f
ed67482
a75cb2c
c31eac4
6d00de6
6ed76ae
6fa4098
ad4a7b1
d1552ee
8ca97c7
c60b851
8a0067d
3614bd6
5787e95
a0ddad9
4b84de3
26867f1
45ffade
b72e4be
1f0e5f9
0075a6e
629b995
a6279dc
5da8f3c
9567f05
0ede82b
b3e962a
871ba72
55c9980
be8e590
3a0c74e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,7 @@ | |
package service | ||
|
||
import ( | ||
"errors" | ||
"fmt" | ||
"strconv" | ||
"strings" | ||
|
@@ -39,7 +40,7 @@ type ConfigurationEditFlags struct { | |
PodSpecFlags knflags.PodSpecFlags | ||
|
||
// Direct field manipulation | ||
Scale int | ||
Scale string | ||
MinScale int | ||
MaxScale int | ||
ConcurrencyTarget int | ||
|
@@ -88,7 +89,7 @@ func (p *ConfigurationEditFlags) addSharedFlags(command *cobra.Command) { | |
command.Flags().MarkHidden("max-scale") | ||
p.markFlagMakesRevision("max-scale") | ||
|
||
command.Flags().IntVar(&p.Scale, "scale", 0, "Minimum and maximum number of replicas.") | ||
command.Flags().StringVar(&p.Scale, "scale", "1", "Set the Minimum and Maximum number of replicas. You can use this flag to set both to a single value, or set a range with min/max values, or set either min or max values without specifying the other. Example: --scale 5 or --scale 1..5 or --scale 1.. or --scale ..5.") | ||
p.markFlagMakesRevision("scale") | ||
|
||
command.Flags().IntVar(&p.MinScale, "scale-min", 0, "Minimum number of replicas.") | ||
|
@@ -358,11 +359,15 @@ func (p *ConfigurationEditFlags) Apply( | |
} else if cmd.Flags().Changed("scale-min") { | ||
return fmt.Errorf("only --scale or --scale-min can be specified") | ||
} else { | ||
err = servinglib.UpdateMaxScale(template, p.Scale) | ||
scaleMin, scaleMax, err := p.scaleConversion(p.Scale) | ||
if err != nil { | ||
return err | ||
} | ||
err = servinglib.UpdateMinScale(template, p.Scale) | ||
err = servinglib.UpdateMaxScale(template, scaleMax) | ||
if err != nil { | ||
return err | ||
} | ||
err = servinglib.UpdateMinScale(template, scaleMin) | ||
if err != nil { | ||
return err | ||
} | ||
|
@@ -560,3 +565,33 @@ func (p *ConfigurationEditFlags) AnyMutation(cmd *cobra.Command) bool { | |
} | ||
return false | ||
} | ||
|
||
// Helper function for --scale | ||
func (p *ConfigurationEditFlags) scaleConversion(scale string) (scaleMin int, scaleMax int, err error) { | ||
if len(scale) <= 2 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My preference would be to divide this into smaller private functions for each of the cases... might simply things. Also for each case returning early could also simply code since once case identified the result is there. No need for big if/else if/else. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I break up the helper function into multiple private functions it will complicate the cmd.Flags().Changed("scale") section, is that ok? I was trying to minimize how large that code block was going to be. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Working on breaking this up next - I should be able to avoid complication in the cmd.Flags().Changed("scale") section. |
||
if !strings.Contains(scale, "..") { | ||
scaleMin, err = strconv.Atoi(scale) | ||
if err != nil { | ||
return 0, 0, err | ||
} | ||
scaleMax = scaleMin | ||
} | ||
} else if strings.Contains(scale, "..") { | ||
scaleParts := strings.Split(scale, "..") | ||
if scaleParts[0] != "" { | ||
scaleMin, err = strconv.Atoi(scaleParts[0]) | ||
if err != nil { | ||
return 0, 0, err | ||
} | ||
} | ||
if scaleParts[1] != "" { | ||
scaleMax, err = strconv.Atoi(scaleParts[1]) | ||
if err != nil { | ||
return 0, 0, err | ||
} | ||
} | ||
} else { | ||
return 0, 0, errors.New("Scale must be of the format x..y or x") | ||
} | ||
return scaleMin, scaleMax, err | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -618,6 +618,116 @@ func TestServiceCreateScaleWithMinScaleSet(t *testing.T) { | |
|
||
} | ||
|
||
func TestServiceCreateScaleRange(t *testing.T) { | ||
action, created, _, err := fakeServiceCreate([]string{ | ||
"service", "create", "foo", "--image", "gcr.io/foo/bar:baz", | ||
"--scale", "1..5", "--no-wait"}, false) | ||
|
||
if err != nil { | ||
t.Fatal(err) | ||
} else if !action.Matches("create", "services") { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would separate them as two different checks. No need to be if/else since either execution will stop and fail the test. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I separated these, however the original blocks were copied from other tests and I was trying to keep it consistent. Many of the other code blocks in create_test and update_test use:
Should I revert this back for consistency? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if this is the case, then typically I would recommend to move the shared code pattern into a dedicated function for validation like in a However as this is not in scope for this pr I would recommend to do this in a separate pr and for now just keep the pattern so that it can be easily detected when doing the full refactoring. |
||
t.Fatalf("Bad action %v", action) | ||
} | ||
|
||
template := &created.Spec.Template | ||
|
||
actualAnnos := template.Annotations | ||
expectedAnnos := []string{ | ||
"autoscaling.knative.dev/minScale", "1", | ||
"autoscaling.knative.dev/maxScale", "5", | ||
} | ||
|
||
for i := 0; i < len(expectedAnnos); i += 2 { | ||
anno := expectedAnnos[i] | ||
if actualAnnos[anno] != expectedAnnos[i+1] { | ||
t.Fatalf("Unexpected annotation value for %s : %s (actual) != %s (expected)", | ||
anno, actualAnnos[anno], expectedAnnos[i+1]) | ||
} | ||
} | ||
} | ||
|
||
func TestServiceCreateScaleRangeOnlyMin(t *testing.T) { | ||
action, created, _, err := fakeServiceCreate([]string{ | ||
"service", "create", "foo", "--image", "gcr.io/foo/bar:baz", | ||
"--scale", "1..", "--no-wait"}, false) | ||
|
||
if err != nil { | ||
t.Fatal(err) | ||
} else if !action.Matches("create", "services") { | ||
t.Fatalf("Bad action %v", action) | ||
} | ||
|
||
template := &created.Spec.Template | ||
|
||
actualAnnos := template.Annotations | ||
expectedAnnos := []string{ | ||
"autoscaling.knative.dev/minScale", "1", | ||
} | ||
|
||
for i := 0; i < len(expectedAnnos); i += 2 { | ||
anno := expectedAnnos[i] | ||
if actualAnnos[anno] != expectedAnnos[i+1] { | ||
t.Fatalf("Unexpected annotation value for %s : %s (actual) != %s (expected)", | ||
anno, actualAnnos[anno], expectedAnnos[i+1]) | ||
} | ||
} | ||
} | ||
|
||
func TestServiceCreateScaleRangeOnlyMax(t *testing.T) { | ||
action, created, _, err := fakeServiceCreate([]string{ | ||
"service", "create", "foo", "--image", "gcr.io/foo/bar:baz", | ||
"--scale", "..5", "--no-wait"}, false) | ||
|
||
if err != nil { | ||
t.Fatal(err) | ||
} else if !action.Matches("create", "services") { | ||
t.Fatalf("Bad action %v", action) | ||
} | ||
|
||
template := &created.Spec.Template | ||
|
||
actualAnnos := template.Annotations | ||
expectedAnnos := []string{ | ||
"autoscaling.knative.dev/maxScale", "5", | ||
} | ||
|
||
for i := 0; i < len(expectedAnnos); i += 2 { | ||
anno := expectedAnnos[i] | ||
if actualAnnos[anno] != expectedAnnos[i+1] { | ||
t.Fatalf("Unexpected annotation value for %s : %s (actual) != %s (expected)", | ||
anno, actualAnnos[anno], expectedAnnos[i+1]) | ||
} | ||
} | ||
} | ||
|
||
func TestServiceCreateScaleRangeOnlyMinWrongSeparator(t *testing.T) { | ||
_, _, _, err := fakeServiceCreate([]string{ | ||
"service", "create", "foo", "--image", "gcr.io/foo/bar:baz", | ||
"--scale", "1--", "--no-wait"}, true) | ||
if err == nil { | ||
t.Fatal(err) | ||
} | ||
expectedErrMsg := "Scale must be of the format x..y or x" | ||
if !strings.Contains(err.Error(), expectedErrMsg) { | ||
t.Errorf("Invalid error output, expected: %s, got : '%s'", expectedErrMsg, err) | ||
} | ||
|
||
} | ||
|
||
func TestServiceCreateScaleRangeOnlyMaxWrongSeparator(t *testing.T) { | ||
_, _, _, err := fakeServiceCreate([]string{ | ||
"service", "create", "foo", "--image", "gcr.io/foo/bar:baz", | ||
"--scale", "--1", "--no-wait"}, true) | ||
if err == nil { | ||
t.Fatal(err) | ||
} | ||
expectedErrMsg := "Scale must be of the format x..y or x" | ||
if !strings.Contains(err.Error(), expectedErrMsg) { | ||
t.Errorf("Invalid error output, expected: %s, got : '%s'", expectedErrMsg, err) | ||
} | ||
|
||
} | ||
|
||
func TestServiceCreateRequestsLimitsCPUMemory(t *testing.T) { | ||
action, created, _, err := fakeServiceCreate([]string{ | ||
"service", "create", "foo", "--image", "gcr.io/foo/bar:baz", | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not use a “.” in the last example “or —scale ..5” since it’s confusing as to whether that last character is part of the syntax
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with each example listed we can add what does that pattern represent in
()
? like'--scale 1..' (min-scale=1, max scale undefined)
and also we should single quote the range patternsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. I'll make this update - have been working on breaking up the helper function at the moment. This will get added to my next commit. I've also updated the formatting so that it looks like other examples in the configuration_edit_flags.go file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for putting literal examples in quotes, but i would suggest double quotes (") as in the example for
--revision-name
(just to be consistent).