Added the option to use --scale for setting MinScale and MaxScale to the same value #914

mpetason · 2020-07-02T17:21:07Z

Description

This allows a user to set --scale which will update MinScale and MaxScale to the same value.

Changes

add --scale as a CLI option

Reference

#822

mpetason · 2020-07-02T18:27:11Z

/retest

test/e2e/service_options_test.go

maximilien

At least address the e2e test change and the coverage if possible. Thanks.

maximilien · 2020-07-07T00:38:37Z

docs/cmd/kn_service_update.md

@@ -71,6 +71,7 @@ kn service update NAME
      --requests-cpu string           DEPRECATED: please use --request instead. The requested CPU (e.g., 250m).
      --requests-memory string        DEPRECATED: please use --request instead. The requested memory (e.g., 64Mi).
      --revision-name string          The revision name to set. Must start with the service name and a dash as a prefix. Empty revision name will result in the server generating a name for the revision. Accepts golang templates, allowing {{.Service}} for the service name, {{.Generation}} for the generation, and {{.Random [n]}} for n random consonants. (default "{{.Service}}-{{.Random 5}}-{{.Generation}}")
+      --scale int                     Minimal and Maximal number of replicas.


Wonder if “Minimum” and “maximum” are better words here?

👍 (if I look at the result of Google hits for "minimum number" and "minimal number"). Also please use lower case, something like

"Minimum and maximum number of replicas"

I had "Set min and max scale as the same value" but something recommended those changes when I submitted the first PR? I'll make this update.

maximilien · 2020-07-07T00:45:12Z

pkg/kn/commands/service/configuration_edit_flags.go

@@ -434,6 +438,17 @@ func (p *ConfigurationEditFlags) Apply(
 		}
 	}

+	if cmd.Flags().Changed("scale") {


I can comment lines 443-445 and/or lines 447-449 and all tests still pass.

This tells me you don’t have tests for servinglib.UpdateMaxScale(...) and servinglib.UpdateMinScale(...) failings. Might be easy to cover with a test with negative or 0 scale (perhaps).

maximilien · 2020-07-07T00:46:28Z

test/e2e/service_options_test.go

@@ -72,6 +72,11 @@ func TestServiceOptions(t *testing.T) {
 	validateServiceMinScale(r, "svc2", "1")
 	validateServiceMaxScale(r, "svc2", "3")

+	t.Log("create and validate service with scale options ")
+	serviceCreateWithOptions(r, "svc2a", "--scale", "5")


I wonder if you want to try to change scale value for both direction up and down... Thanks.

navidshaikh

@mpetason : single value input works fine, do you plan to add range support or in a separate PR?

rhuss

Looks good, thanks !

However, I don't think it fixes #822 as it only covers the case when you want to set max == min, but #822 also discusses to allow to specify the full range with only --scale (so that we can deprecate --min-scale and --max-scale later).

Happy to merge this now (as the PR implements a subset of the full solution), but then we should open another issue for tracking the extended syntax.

rhuss · 2020-07-07T08:21:08Z

pkg/kn/commands/service/configuration_edit_flags.go

@@ -185,6 +186,9 @@ func (p *ConfigurationEditFlags) addSharedFlags(command *cobra.Command) {
 	command.Flags().IntVar(&p.MaxScale, "max-scale", 0, "Maximal number of replicas.")
 	p.markFlagMakesRevision("max-scale")

+	command.Flags().IntVar(&p.Scale, "scale", 0, "Minimal and Maximal number of replicas.")


Suggested change

command.Flags().IntVar(&p.Scale, "scale", 0, "Minimal and Maximal number of replicas.")

command.Flags().IntVar(&p.Scale, "scale", 0, "Minimum and maximum number of replicas.")

rhuss · 2020-07-07T08:23:54Z

pkg/kn/commands/service/create_test.go

+	action, created, _, err := fakeServiceCreate([]string{
+		"service", "create", "foo", "--image", "gcr.io/foo/bar:baz",
+		"--scale", "5",
+		"--concurrency-target", "10", "--concurrency-limit", "100",


Why are these options important here ? I would just focus on --scale to make this test more focussed and easier to maintain (e.g. when something around concurrency-target would change in the future)

As mentioned by @maximilien you should also add a test for the error case, i.e. when --scale is negative.

And also a test for what happens when --scale and --min-scale & --max-scale are all provided (the more specific options should take precedence or an error should be thrown if both set of options are used)

I will make these updates. This was just copying the test for min/max scale above it.

rhuss · 2020-07-07T08:24:43Z

pkg/kn/commands/service/update_test.go

+
+	action, updated, _, err := fakeServiceUpdate(original, []string{
+		"service", "update", "foo",
+		"--scale", "5", "--concurrency-target", "10", "--concurrency-limit", "100", "--concurrency-utilization", "50", "--no-wait"})


Same as above, please only test for --scale

rhuss

Please update the test and wording of the CLI option.

rhuss · 2020-07-07T08:32:04Z

pkg/kn/commands/service/configuration_edit_flags.go

@@ -434,6 +438,17 @@ func (p *ConfigurationEditFlags) Apply(
 		}
 	}

+	if cmd.Flags().Changed("scale") {
+		err = servinglib.UpdateMaxScale(template, p.Scale)


What is if --max-scale is also given on the command line ? Which one takes precedence ? Or should an error be thrown ?

IMO we should return an error if both --scale and either --min-scale or --max-scale is provided.

I need to add this functionality and also tests for it.

mpetason · 2020-07-07T18:10:09Z

@rhuss this doesn't "fix" the other issue, but I'm not sure how to remove the "fixes" unless I can just edit the comment. I will update this to range in another PR, just wanted to get the functionality in and learn how the client works with this PR.

mpetason · 2020-07-07T18:14:08Z

Still work in progress. I need to address the @maximilien issue + testing.

same value

annotation

knative-metrics-robot · 2020-07-10T17:03:05Z

The following is the coverage report on the affected files.
Say /test pull-knative-client-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/kn/commands/service/configuration_edit_flags.go	84.6%	85.0%	0.4

mpetason · 2020-07-10T17:36:30Z

I think I have addressed all of the comments/change requests. Ready for another review.

maximilien

Overall looks good to me. Holding to give others a chance to review of unhold

/hold
/lgtm

rhuss

Thanks !

/lgtm
/unhold
/approve

knative-prow-robot · 2020-07-11T08:51:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mpetason, navidshaikh, rhuss

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [navidshaikh,rhuss]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jul 2, 2020

knative-prow-robot requested review from cppforlife and mattmoor July 2, 2020 17:21

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 2, 2020

navidshaikh requested changes Jul 6, 2020

View reviewed changes

test/e2e/service_options_test.go Show resolved Hide resolved

maximilien suggested changes Jul 7, 2020

View reviewed changes

navidshaikh approved these changes Jul 7, 2020

View reviewed changes

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 7, 2020

rhuss approved these changes Jul 7, 2020

View reviewed changes

rhuss requested changes Jul 7, 2020

View reviewed changes

rhuss reviewed Jul 7, 2020

View reviewed changes

knative-prow-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 8, 2020

Michael Petersen added 5 commits July 8, 2020 09:03

Added the option to use --scale for setting MinScale and MaxScale to the

f14d3fa

same value

Updated service create/update to resolve test issues

60c2b75

Removed scale from the annotation section - there isn't a scale

622031f

annotation

Renamed test service name so that it doesn't match a previous test

0906503

Addressed most issues/changes

d195306

knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 8, 2020

Added tests for multiple flags being used at the same time

07ff459

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 9, 2020

Michael Petersen added 2 commits July 9, 2020 11:32

Cleaned up the update tests

17303e0

Added negative value tests and cleaned up tests in create_test.go

54d25f2

maximilien reviewed Jul 10, 2020

View reviewed changes

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 10, 2020

knative-prow-robot assigned maximilien Jul 10, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 10, 2020

rhuss reviewed Jul 11, 2020

View reviewed changes

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 11, 2020

knative-prow-robot assigned rhuss Jul 11, 2020

knative-prow-robot merged commit e1c48e6 into knative:master Jul 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added the option to use --scale for setting MinScale and MaxScale to the same value #914

Added the option to use --scale for setting MinScale and MaxScale to the same value #914

mpetason commented Jul 2, 2020 •

edited

Loading

mpetason commented Jul 2, 2020

maximilien left a comment

maximilien Jul 7, 2020

rhuss Jul 7, 2020

mpetason Jul 7, 2020

maximilien Jul 7, 2020

maximilien Jul 7, 2020

navidshaikh left a comment

rhuss left a comment

rhuss Jul 7, 2020

rhuss Jul 7, 2020

rhuss Jul 7, 2020

rhuss Jul 7, 2020

mpetason Jul 7, 2020

rhuss Jul 7, 2020

rhuss left a comment

rhuss Jul 7, 2020 •

edited

Loading

mpetason Jul 7, 2020

mpetason commented Jul 7, 2020

mpetason commented Jul 7, 2020

knative-metrics-robot commented Jul 10, 2020

mpetason commented Jul 10, 2020

maximilien left a comment

rhuss left a comment

knative-prow-robot commented Jul 11, 2020

	command.Flags().IntVar(&p.Scale, "scale", 0, "Minimal and Maximal number of replicas.")
	command.Flags().IntVar(&p.Scale, "scale", 0, "Minimum and maximum number of replicas.")

Added the option to use --scale for setting MinScale and MaxScale to the same value #914

Added the option to use --scale for setting MinScale and MaxScale to the same value #914

Conversation

mpetason commented Jul 2, 2020 • edited Loading

Description

Changes

Reference

mpetason commented Jul 2, 2020

maximilien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

navidshaikh left a comment

Choose a reason for hiding this comment

rhuss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhuss left a comment

Choose a reason for hiding this comment

rhuss Jul 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpetason commented Jul 7, 2020

mpetason commented Jul 7, 2020

knative-metrics-robot commented Jul 10, 2020

mpetason commented Jul 10, 2020

maximilien left a comment

Choose a reason for hiding this comment

rhuss left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Jul 11, 2020

mpetason commented Jul 2, 2020 •

edited

Loading

rhuss Jul 7, 2020 •

edited

Loading