Add range options to --scale #1070

mpetason · 2020-10-22T16:01:12Z

Description

Adds range options to --scale

Changes

Changed Scale from Int to String
Added Options to set min/max with 1..5

Reference

Fixes #822

Replaces previous PR where I had issues with a rebase - #986

mpetason · 2020-10-22T16:39:27Z

I'm not sure about the failed lint test - maybe I can get more information in a review.

When setting 1.. or ..5 it defaults to 0 <-- I still need to resolve this. I'll write an additional test to cover this as well.

maximilien

Missing a test case for case when user only specifies separator or has too many separators, e.g.,

—scale ..
—scale 1..5..

Also couple more minor feedback. Thanks for contribution.

maximilien · 2020-10-22T21:33:52Z

docs/cmd/kn_service_create.md

@@ -88,7 +88,7 @@ kn service create NAME --image IMAGE
      --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                         Minimum and maximum number of replicas.
+      --scale string                      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. (default "1")


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

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 patterns

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.

+1 for putting literal examples in quotes, but i would suggest double quotes (") as in the example for --revision-name (just to be consistent).

maximilien · 2020-10-22T21:47:31Z

pkg/kn/commands/service/create_test.go

+
+	if err != nil {
+		t.Fatal(err)
+	} else if !action.Matches("create", "services") {


I would separate them as two different checks. No need to be if/else since either execution will stop and fail the test.

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:

if err != nil { t.Fatal(err) } else if !action.Matches("create", "services") { t.Fatalf("Bad action %v", action) }

Should I revert this back for consistency?

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 validateAction(t, action, err, []string{ "create", "services") to guarantee that the same code is used everywhere.

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.

maximilien · 2020-10-22T21:50:51Z

pkg/kn/commands/service/configuration_edit_flags.go

+
+// Helper function for --scale
+func (p *ConfigurationEditFlags) scaleConversion(scale string) (scaleMin int, scaleMax int, err error) {
+	if len(scale) <= 2 {


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.

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.

Working on breaking this up next - I should be able to avoid complication in the cmd.Flags().Changed("scale") section.

maximilien · 2020-10-22T21:53:33Z

/ok-to-test
/approve

knative-prow-robot · 2020-10-22T21:53:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maximilien, mpetason

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~~ [maximilien]

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

Signed-off-by: Knative Automation <[email protected]>

* fix(tekton e2e): Fix CONTAINER_REGISTRY name generation var `E2E_BASE_NAME` is no longer available * debug: Run tekton tests * Revert "debug: Run tekton tests" This reverts commit dbe125e.

mpetason · 2020-10-26T17:31:04Z

TODO:
Break up helper function and update cmd.Flags() section.
Fix issues where undefined value defaults to 0.

Fixed:
Addressed the easier updates based on reviews.

rhuss · 2020-10-27T08:34:17Z

@mpetason please let me know if I can jump on a final review.

…dspec_helper (#1024) * moving utilities of handling podspec to podspec_helper * address comments * refactor code base * change the input parameter from ccmd to flagset * remove comments and add CHANGELOG

…rom max then min, to min then max, updated tests to reflect updates

mpetason · 2020-10-27T20:37:23Z

@rhuss How can I resolve this issue? This is mostly a copy/paste from other tests. Do we need to create a function for this and just have each test call it to reduce the amount of times it's duplicated?

pkg/kn/commands/service/update_test.go#L529

[Redundant Format] reported by reviewdog 🐶
Please consider avoiding tail format like this:
t.Fatalf("Bad action %v", action)

Raw Output:
./pkg/kn/commands/service/update_test.go:529: Please consider avoiding tail format like this:
t.Fatalf("Bad action %v", action)

duglin · 2020-10-29T02:10:02Z

docs/cmd/kn_service_create.md

@@ -88,7 +88,7 @@ kn service create NAME --image IMAGE
      --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                         Minimum and maximum number of replicas.
+      --scale string                      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 (scale-min = 5, scale-max = 5) or --scale 1..5 (scale-min = 1, scale-max = 5) or --scale 1.. (scale-min = 1, scale-max = undefined) or --scale ..5 (scale-min = undefined, scale-max = 5) (default "1")


I think saying “undefined” isn’t as clear as saying that the current value is unchanged. And there is no default value so I would remove that.

I don't think that is is "unchanged" as we are here talking about a create operation. Also for an update operation I still think that --scale 1.. mean min-scale is set to one, but no max-scale is set or removed if set, so that there is no upper bound specified by the user (regardless what was specified before). If you want to change only the minimal scale, you should use --min-scale.

+1 that the default value 1 is wrong as there is supposed to be no default at all, the argument is mandatory.

So maybe instead of scale-max = undefined, better use no maximum scale defined) ?

I'm confused. @rhuss in the past I think you said you wanted 1.. to mean only the lower bound is set, and upper bound is untouched. That's why I commented the way I did - I thought you still wanted that.

Let's back up a bit... at one point people wanted to deprecate --scale-min and only have --scale - is that still in the cards or is that idea dead? If --scale-min/max will live on, then I'm ok with a missing value in .. meaning unset that value (or in the 'create' case it means just leave it as the default). And yes --scale .. would mean unset both :-)

Signed-off-by: Knative Automation <[email protected]>

mpetason · 2020-10-29T16:51:32Z

Split up the helper function, still working on getting the correct values set when something isn't defined. Right now it sets the default to 0 when it isn't set, which isn't what we want for something like "..5" when using "update."

… resolved a couple bugs

mpetason · 2020-11-06T17:10:07Z

/test pull-knative-client-build-tests

mpetason · 2020-11-06T18:24:07Z

Running build again doesn't update the docs, not sure how to resolve the current failing test.

maximilien

Flake perhaps.
/retest

rhuss · 2020-11-09T08:43:14Z

@mpetason You need to rebase you local branch so that you get the newly introduced kn service apply also locally and the regenerate the docs with hack/build.sh. The reason for the build failure is, that the CI did the rebase but you did not when committing, and as kn service apply use also the scale options, it is affected by the update.

Just try a

git pull upstream master
git rebase upstream/master
hack/build.sh
git status
git commit -m "Updated command documentation" -a

knative-metrics-robot · 2020-11-09T08:47:07Z

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	87.5%	86.7%	-0.8

knative-prow-robot · 2020-11-09T08:48:26Z

@mpetason: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-knative-client-build-tests	`4b84de3`	link	`/test pull-knative-client-build-tests`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

* Add channel sink prefix * Add unit and e2e tests * Add CHANGELOG * Fix e2e tests

in the favor of --no-wait flag

* feat: Add service import command * fix: Fix e2e test * fix: Add retry when retrieving Configuration * fix: Reflect review feedback * fix: Fix error message Co-authored-by: Roland Huß <[email protected]> * fix: Add missing mock tests * fix: Polish unit test assertions * fix: Mark import as experimental * chore: Add changelog entry * Update CHANGELOG.adoc Co-authored-by: Roland Huß <[email protected]> * fix: Remove deprecated flag Co-authored-by: Roland Huß <[email protected]>

…rom max then min, to min then max, updated tests to reflect updates

… resolved a couple bugs

…-822-pt3

mpetason · 2020-11-09T19:17:24Z

@rhuss that didn't work, I guess I'll close this issue and try again.

Add range options to --scale

3db3cdb

google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Oct 22, 2020

knative-prow-robot requested review from cppforlife and rhuss October 22, 2020 16:01

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 22, 2020

Mike Petersen added 2 commits October 22, 2020 09:03

Updated scale flag so that the docs update with the correct information

7f91820

genereted docs instead of updating manually

2edf4c4

maximilien suggested changes Oct 22, 2020

View reviewed changes

knative-prow-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Oct 22, 2020

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

knative-automation and others added 6 commits October 25, 2020 22:10

Update common github actions (#1072)

84eb376

Signed-off-by: Knative Automation <[email protected]>

fix(tekton e2e): Fix CONTAINER_REGISTRY name generation (#1074)

abb75e4

* fix(tekton e2e): Fix CONTAINER_REGISTRY name generation var `E2E_BASE_NAME` is no longer available * debug: Run tekton tests * Revert "debug: Run tekton tests" This reverts commit dbe125e.

Updated --scale docs

2b7631a

Broke up if/else into two if statements in tests

33d9193

Added missing ")" in scale docs

efa9e27

Generated docs after updating configuration_edit_flags

537973f

Ying Chun Guo and others added 2 commits October 27, 2020 10:45

Add ResolvePodSpec to podspec.go and move the related utilities to po…

ed67482

…dspec_helper (#1024) * moving utilities of handling podspec to podspec_helper * address comments * refactor code base * change the input parameter from ccmd to flagset * remove comments and add CHANGELOG

Resolved setting either value to 0, re-ordered which gets set first f…

a75cb2c

…rom max then min, to min then max, updated tests to reflect updates

Fixing lint issues

c31eac4

duglin reviewed Oct 29, 2020

View reviewed changes

knative-automation and others added 3 commits October 28, 2020 19:38

Update common github actions (#1081)

6d00de6

Signed-off-by: Knative Automation <[email protected]>

uniform multiple writeSink into DescribeSink (#1075)

6ed76ae

Split up helper function, updated wording in help section

6fa4098

got rid of helper functions, cleaned up code so it is easier to read,…

4b84de3

… resolved a couple bugs

maximilien reviewed Nov 6, 2020

View reviewed changes

rhuss closed this Nov 9, 2020

rhuss reopened this Nov 9, 2020

navidshaikh and others added 15 commits November 9, 2020 01:49

feat: Add channel sink prefix (#1092)

26867f1

* Add channel sink prefix * Add unit and e2e tests * Add CHANGELOG * Fix e2e tests

Drop deprecated --async flag support (#1094)

45ffade

in the favor of --no-wait flag

Add range options to --scale

1f0e5f9

Updated scale flag so that the docs update with the correct information

0075a6e

genereted docs instead of updating manually

629b995

Updated --scale docs

a6279dc

Broke up if/else into two if statements in tests

5da8f3c

Added missing ")" in scale docs

9567f05

Generated docs after updating configuration_edit_flags

0ede82b

Resolved setting either value to 0, re-ordered which gets set first f…

b3e962a

…rom max then min, to min then max, updated tests to reflect updates

Fixing lint issues

871ba72

Split up helper function, updated wording in help section

55c9980

got rid of helper functions, cleaned up code so it is easier to read,…

be8e590

… resolved a couple bugs

Merge branch 'issue-822-pt3' of github.com:mpetason/client into issue…

3a0c74e

…-822-pt3

knative-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 9, 2020

mpetason closed this Nov 9, 2020

mpetason deleted the issue-822-pt3 branch November 11, 2020 16:20

mpetason mentioned this pull request Nov 11, 2020

adds --scale range option #1114

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add range options to --scale #1070

Add range options to --scale #1070

mpetason commented Oct 22, 2020

mpetason commented Oct 22, 2020

maximilien left a comment

maximilien Oct 22, 2020

navidshaikh Oct 26, 2020 •

edited

Loading

mpetason Oct 26, 2020

rhuss Oct 30, 2020

maximilien Oct 22, 2020

mpetason Oct 27, 2020

rhuss Oct 30, 2020

maximilien Oct 22, 2020

mpetason Oct 26, 2020

mpetason Oct 28, 2020

maximilien commented Oct 22, 2020

knative-prow-robot commented Oct 22, 2020

mpetason commented Oct 26, 2020

rhuss commented Oct 27, 2020

mpetason commented Oct 27, 2020

duglin Oct 29, 2020

rhuss Oct 30, 2020

rhuss Oct 30, 2020

duglin Oct 30, 2020

mpetason commented Oct 29, 2020

mpetason commented Nov 6, 2020

mpetason commented Nov 6, 2020

maximilien left a comment

rhuss commented Nov 9, 2020 •

edited

Loading

knative-metrics-robot commented Nov 9, 2020

knative-prow-robot commented Nov 9, 2020

mpetason commented Nov 9, 2020

Add range options to --scale #1070

Add range options to --scale #1070

Conversation

mpetason commented Oct 22, 2020

Description

Changes

Reference

mpetason commented Oct 22, 2020

maximilien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

navidshaikh Oct 26, 2020 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maximilien commented Oct 22, 2020

knative-prow-robot commented Oct 22, 2020

mpetason commented Oct 26, 2020

rhuss commented Oct 27, 2020

mpetason commented Oct 27, 2020

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

mpetason commented Oct 29, 2020

mpetason commented Nov 6, 2020

mpetason commented Nov 6, 2020

maximilien left a comment

Choose a reason for hiding this comment

rhuss commented Nov 9, 2020 • edited Loading

knative-metrics-robot commented Nov 9, 2020

knative-prow-robot commented Nov 9, 2020

mpetason commented Nov 9, 2020

navidshaikh Oct 26, 2020 •

edited

Loading

rhuss commented Nov 9, 2020 •

edited

Loading