[WIP] Added range options to --scale #986

mpetason · 2020-08-20T18:46:35Z

Description

Adds range options to --scale.

Changes

Adds three more options to --scale (1..5, 1.., ..5)

Reference

Fixes #822

mpetason · 2020-08-20T18:46:44Z

Work in Progress. Plan on adding a helper function + more tests:

Test 0..5
Test 0..
Test ..0
Test 1,,5 (using incorrect separation)

maximilien · 2020-08-20T20:55:02Z

/ok-to-test

Ready for some reviews @mpetason?

mpetason · 2020-08-20T21:17:23Z

Not ready for a review yet. I need to cleanup the range section and create a helper function. Right now it's just in an "it works" status and I wanted to show some work.

maximilien

Some initial feedback. Also, make sure to add a e2e test.

maximilien · 2020-08-20T21:41:09Z

docs/cmd/kn_service_create.md

@@ -86,7 +86,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                  Minimum and maximum number of replicas. Example: --scale 5 or --scale 1..5 or --scale 1.. or --scale ..5.


What does it mean to do —scale ..5 (up to five?). Maybe lets be clear with text...

maximilien · 2020-08-20T21:41:42Z

pkg/kn/commands/service/configuration_edit_flags.go

@@ -82,7 +83,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", "", "Minimum and maximum number of replicas.")


Is “” a valid value? Perhaps use default?

maximilien · 2020-08-20T21:43:15Z

pkg/kn/commands/service/configuration_edit_flags.go

-			if err != nil {
-				return err
+			if !strings.Contains(p.Scale, "..") {
+				scaleInt, _ := strconv.Atoi(p.Scale)


Why are you ignoring the Atoi(...) error? Just catch and check it

maximilien · 2020-08-20T21:43:32Z

pkg/kn/commands/service/configuration_edit_flags.go

+				}
+			} else if len(p.Scale) == 4 {
+				scaleParts := strings.Split(p.Scale, "..")
+				scaleMin, _ := strconv.Atoi(scaleParts[0])


Same here... don’t ignore errors

maximilien · 2020-08-21T21:14:11Z

Not ready for a review yet. I need to cleanup the range section and create a helper function. Right now it's just in an "it works" status and I wanted to show some work.

OK, sorry. I'll do another pass when ready. Just tag me and remove the WIP label. Cheers 🍻

rhuss · 2020-08-25T10:52:52Z

fyi, the fix for the failing CI is included as part of #902

rhuss · 2020-08-25T12:59:17Z

/retest

maximilien · 2020-08-25T21:59:32Z

Wonder if this is a flake

/test pull-knative-client-build-tests

rhuss · 2020-09-01T07:44:23Z

@mpetason you would need to recreate the docs via build.sh and commit the generated/updated markdown files

mpetason · 2020-09-01T14:22:49Z

@rhuss will do. I should have an update for this today.

mpetason · 2020-09-01T22:45:20Z

Still WIP, need to update tests and fix how I'm checking for errors.

lgtm-com · 2020-09-02T17:43:31Z

This pull request introduces 1 alert when merging f9e53d5 into c801e82 - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

mpetason · 2020-09-02T18:39:24Z

@maximilien @rhuss I think I'm ready for a review now. I need to update the docs (reverted back with the last build), but would like some pointers on changes I can make to the code/tests.

daisy-ycguo

Thank you for the contributions. I think this PR looks good generally. I just left a few comments.

daisy-ycguo · 2020-09-14T05:36:28Z

pkg/kn/commands/service/configuration_edit_flags.go

@@ -82,7 +84,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", "Minimum and maximum number of replicas.")


I suggest to add a sample at the end of the explanation, e.g.

Minimum and maximum number of replicas. (e.g., 1..5).

in order to help end users understand the format clearly.

daisy-ycguo · 2020-09-14T05:41:34Z

pkg/kn/commands/service/configuration_edit_flags.go

@@ -480,3 +486,32 @@ func (p *ConfigurationEditFlags) AnyMutation(cmd *cobra.Command) bool {
 	}
 	return false
 }
+
+func (p *ConfigurationEditFlags) ScaleConversion(scale string) (scaleMin int, scaleMax int, err error) {


Do you want the method ScaleConversion to be visible outside package? If the method is not used by any other packages, I suggest to change the method name to start with a lower case letter, e.g. scaleConversion

mpetason · 2020-09-16T14:13:06Z

/restest

mpetason · 2020-09-21T21:06:41Z

I didn't rebase this correctly and have no clue how to fix it. I guess I need to close/delete this PR and try again?

knative-metrics-robot · 2020-09-21T21:08:39Z

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	83.2%	83.4%	0.2

knative-prow-robot · 2020-10-01T16:48:01Z

@mpetason: PR needs rebase.

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.

knative-prow-robot · 2020-10-01T16:48:01Z

@mpetason: PR needs rebase.

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.

mpetason · 2020-10-01T16:54:07Z

Can't figure out how to fix the issue where there are so many files added. Tried a rebase but it doesn't look like that helped. Going to close this out for good and open a new PR with the same work.

Fixed issues related to reviews and cleaned up some of the logic Removed error checking - will add back after more updates Ran build to update docs/created a helper script Updated helper function/cleaned it up and resolved failing tests Added checks for non ".." values and added more tests Updated wording and scaleconversion Added range options to --scale Fixed issues related to reviews and cleaned up some of the logic Removed error checking - will add back after more updates Ran build to update docs/created a helper script Updated helper function/cleaned it up and resolved failing tests Added checks for non ".." values and added more tests Updated wording and scaleconversion Added range options to --scale Fixed issues related to reviews and cleaned up some of the logic Ran build to update docs/created a helper script Updated helper function/cleaned it up and resolved failing tests Updated wording and scaleconversion Added range options to --scale Fixed issues related to reviews and cleaned up some of the logic Removed error checking - will add back after more updates Ran build to update docs/created a helper script Updated helper function/cleaned it up and resolved failing tests Added checks for non ".." values and added more tests Updated wording and scaleconversion Added comment about the helper function

knative-prow-robot · 2020-10-07T14:04:04Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mpetason
To complete the pull request process, please assign sixolet after the PR has been reviewed.
You can assign the PR to them by writing /assign @sixolet in a comment when ready.

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

Needs approval from an approver in each of these files:

OWNERS

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

knative-prow-robot · 2020-10-07T14:16:35Z

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

Test name	Commit	Details	Rerun command
pull-knative-client-integration-tests-latest-release	`0bc3b80`	link	`/test pull-knative-client-integration-tests-latest-release`
pull-knative-client-unit-tests	`0bc3b80`	link	`/test pull-knative-client-unit-tests`
pull-knative-client-build-tests	`0bc3b80`	link	`/test pull-knative-client-build-tests`
pull-knative-client-integration-tests	`0bc3b80`	link	`/test pull-knative-client-integration-tests`
pull-knative-client-go-coverage	`0bc3b80`	link	`/test pull-knative-client-go-coverage`

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.

mpetason · 2020-10-07T14:20:06Z

Figured out how to clean this up after rebase. I'm probably going to close this out and re-open since I'm still running into conflicts with the command information files.

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

mpetason changed the title ~~Added range options to --scale~~ [WIP] Added range options to --scale Aug 20, 2020

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

maximilien suggested changes Aug 20, 2020

View reviewed changes

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

daisy-ycguo suggested changes Sep 14, 2020

View reviewed changes

mpetason closed this Sep 21, 2020

mpetason reopened this Oct 1, 2020

knative-prow-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 1, 2020

mpetason closed this Oct 1, 2020

mpetason reopened this Oct 7, 2020

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

cleaned up after rebase

0bc3b80

mpetason closed this Oct 15, 2020

mpetason deleted the issue-822-pt3 branch October 22, 2020 15:31

mpetason restored the issue-822-pt3 branch October 22, 2020 15:31

mpetason deleted the issue-822-pt3 branch October 22, 2020 15:32

mpetason mentioned this pull request Oct 22, 2020

Add range options to --scale #1070

Closed

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

[WIP] Added range options to --scale #986

[WIP] Added range options to --scale #986

mpetason commented Aug 20, 2020

mpetason commented Aug 20, 2020 •

edited

Loading

maximilien commented Aug 20, 2020

mpetason commented Aug 20, 2020

maximilien left a comment

maximilien Aug 20, 2020

maximilien Aug 20, 2020

maximilien Aug 20, 2020

maximilien Aug 20, 2020

maximilien commented Aug 21, 2020

rhuss commented Aug 25, 2020

rhuss commented Aug 25, 2020

maximilien commented Aug 25, 2020

rhuss commented Sep 1, 2020

mpetason commented Sep 1, 2020

mpetason commented Sep 1, 2020

lgtm-com bot commented Sep 2, 2020

mpetason commented Sep 2, 2020

daisy-ycguo left a comment

daisy-ycguo Sep 14, 2020

daisy-ycguo Sep 14, 2020

mpetason commented Sep 16, 2020

mpetason commented Sep 21, 2020

knative-metrics-robot commented Sep 21, 2020

knative-prow-robot commented Oct 1, 2020

knative-prow-robot commented Oct 1, 2020

mpetason commented Oct 1, 2020

knative-prow-robot commented Oct 7, 2020

knative-prow-robot commented Oct 7, 2020

mpetason commented Oct 7, 2020

[WIP] Added range options to --scale #986

[WIP] Added range options to --scale #986

Conversation

mpetason commented Aug 20, 2020

Description

Changes

Reference

mpetason commented Aug 20, 2020 • edited Loading

maximilien commented Aug 20, 2020

mpetason commented Aug 20, 2020

maximilien left a comment

Choose a reason for hiding this comment

maximilien Aug 20, 2020

Choose a reason for hiding this comment

maximilien Aug 20, 2020

Choose a reason for hiding this comment

maximilien Aug 20, 2020

Choose a reason for hiding this comment

maximilien Aug 20, 2020

Choose a reason for hiding this comment

maximilien commented Aug 21, 2020

rhuss commented Aug 25, 2020

rhuss commented Aug 25, 2020

maximilien commented Aug 25, 2020

rhuss commented Sep 1, 2020

mpetason commented Sep 1, 2020

mpetason commented Sep 1, 2020

lgtm-com bot commented Sep 2, 2020

mpetason commented Sep 2, 2020

daisy-ycguo left a comment

Choose a reason for hiding this comment

daisy-ycguo Sep 14, 2020

Choose a reason for hiding this comment

daisy-ycguo Sep 14, 2020

Choose a reason for hiding this comment

mpetason commented Sep 16, 2020

mpetason commented Sep 21, 2020

knative-metrics-robot commented Sep 21, 2020

knative-prow-robot commented Oct 1, 2020

knative-prow-robot commented Oct 1, 2020

mpetason commented Oct 1, 2020

knative-prow-robot commented Oct 7, 2020

knative-prow-robot commented Oct 7, 2020

mpetason commented Oct 7, 2020

mpetason commented Aug 20, 2020 •

edited

Loading