add autoscaling list command #17

chaozbj · 2020-11-12T13:03:16Z

The issue is: #15
here is an example of output:

12:00 ➜ ~/Workspace/code/github.com/kn-plugin-admin/cmd [chao-15 ✘] ./cmd autoscaling list
NAME                         VALUE
enable-scale-to-zero         false
max-scale-down-rate          3
panic-threshold-percentage   100

pkg/command/autoscaling/list.go

zhanggbj

@chaozbj it's good to add at least one e2e test case for autoscaling list in test/e2e/kn_admin_test.go, thanks!

maximilien

Couple comments. Also need e2e tests. These are the best :)

Otherwise LGTM

maximilien · 2020-11-18T23:11:57Z

pkg/command/autoscaling/list.go

+			currentCm := &corev1.ConfigMap{}
+			currentCm, err := p.ClientSet.CoreV1().ConfigMaps(knativeServing).Get(configAutoscaler, metav1.GetOptions{})
+			if err != nil {
+				return fmt.Errorf("failed to get ConfigMaps: %+v", err)


I wonder if putting the error message here will be useful to user?

maximilien · 2020-11-18T23:12:55Z

pkg/command/autoscaling/list_test.go

+func TestAutoscalingListEmpty(t *testing.T) {
+	t.Run("no flags", func(t *testing.T) {
+		cm := &corev1.ConfigMap{
+			ObjectMeta: metav1.ObjectMeta{


Maybe when it’s empty the result should be “No autoscaling info available” or similar instead of a list with headers...

Thanks @maximilien, I will improve the output for empty configs.

@maximilien and @zhanggbj Thanks, for e2e test case, yes, I missed it, I will add, but I saw @zhanggbj added the test function for autoscaling func (et *e2eTest) knAdminAutoscaling(t *testing.T, r *test.KnRunResultCollector) in her RP #16 , I think I should wait the PR to deliver first and then modify the same function to add tests for list command or I can add it later.

@chaozbj make sense, so let's wait #16 merge first and then you can rebase and add e2e in this PR then.

zhanggbj · 2020-11-20T05:47:42Z

/hold
wait for #16 to merge first.

zhanggbj · 2020-11-27T05:41:45Z

/unhold
#16 is merged, please help to rebase and finish this PR, thanks!

chaozbj · 2020-11-30T02:09:00Z

@zhanggbj @maximilien Changes as below:

Following knative/client v0.17 to update go.mod because I want to use autoscaler.Config.ScaleToZeroPodRetentionPeriod field which is introduced from knative/serving v0.15.

I didn't follow the knative/client v0.18 or v0.19 to update go.mod since I got compiling error when I use k8s.io/apimachinery >= v0.18, there is an interface changed from v0.18(maybe from earlier version):
```
../pkg/command/utils/utils.go:27:72: not enough arguments in call to client.CoreV1().ConfigMaps(desiredCm.ObjectMeta.Namespace).Get
have (string, "k8s.io/apimachinery/pkg/apis/meta/v1".GetOptions)
want (context.Context, string, "k8s.io/apimachinery/pkg/apis/meta/v1".GetOptions)
```
We need to change our codes if we want to adopt it, but I don't want to address them in this PR, so I choosed the older version to use.
Always list all values for autoscaler configs, if the config is changed by user, the user's value will be printed, otherwise the default value will be printed. I called autoscalerconfig.NewConfigFromMap(map[string]string) defined in knative/serving to build configs with default value, that is why I need to update go.mod.
Updated unit test codes for new changes.
Added e2e test case for autoscaling list command.
A minor change for release.sh, using -mod=readonly to replace-mod=vendor because we don't use vendor now.

Please help me to review, thanks!

cmd/release.sh

go.mod

zhanggbj · 2020-11-30T04:01:42Z

@chaozbj Thanks Chao for the efforts! In general, the changes look good to me,

For 1) Let's finish the dependency bump firstly in PR #21, we need it to build release 0.1.0, this will also resolve the issue you mentioned.

For 5) I think ./hack/xx is more proper for some scripts and tools like release.sh

chaozbj · 2020-12-03T14:59:18Z

@zhanggbj I rebased it from PR #25 and also deleted release.sh script because I found that hack/build.sh can do the same thing. Please help me review, thanks!

knative-prow-robot · 2020-12-03T17:12:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chaozbj, maximilien, zhanggbj

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,zhanggbj]

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

zhanggbj · 2020-12-03T17:14:06Z

/lgtm
Look nice and merging! Thanks @chaozbj!

zhanggbj · 2020-12-03T17:20:32Z

Prow tests are failing due to test-infra setting in progress which is covered by #24

/usr/local/bin/kubekins-runner.sh: line 104: ./test/presubmit-tests.sh: No such file or directory

maximilien

Tests still failing
/retest

navidshaikh · 2020-12-04T12:18:37Z

/test all

zhanggbj · 2020-12-04T12:37:55Z

/retest

zhanggbj · 2020-12-04T12:40:45Z

/lgtm cancel

@chaozbj would you please help to rebase first. Just merged Navid's PR for test infra.

Signed-off-by: chaozbj <[email protected]>

chaozbj · 2020-12-04T12:54:56Z

/retest

chaozbj · 2020-12-04T13:06:21Z

/retest

navidshaikh · 2020-12-04T13:13:08Z

@chaozbj : there is a verify codegen step now part of the build tests,


ERROR: /home/prow/go/src/knative.dev/kn-plugin-admin is out of date. Please run ./hack/build.sh -c and commit.
--

please run ./hack/build.sh and commit the changes

Signed-off-by: chaozbj <[email protected]>

knative-prow-robot · 2020-12-04T13:28:07Z

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

Test name	Commit	Details	Rerun command
pull-knative-sandbox-kn-plugin-admin-build-tests	`4e4a2b4`	link	`/test pull-knative-sandbox-kn-plugin-admin-build-tests`
pull-knative-sandbox-kn-plugin-admin-integration-tests	`4e4a2b4`	link	`/test pull-knative-sandbox-kn-plugin-admin-integration-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.

chaozbj · 2020-12-04T13:46:38Z

close it and will create a new PR for autoscaling list command

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

knative-prow-robot requested review from navidshaikh and rhuss November 12, 2020 13:03

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

zhanggbj reviewed Nov 16, 2020

View reviewed changes

pkg/command/autoscaling/list.go Outdated Show resolved Hide resolved

zhanggbj reviewed Nov 16, 2020

View reviewed changes

maximilien approved these changes Nov 18, 2020

View reviewed changes

knative-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Nov 18, 2020

chaozbj mentioned this pull request Nov 30, 2020

Update module dependency to latest in go.mod #21

Closed

zhanggbj reviewed Nov 30, 2020

View reviewed changes

cmd/release.sh Outdated Show resolved Hide resolved

go.mod Outdated Show resolved Hide resolved

chaozbj force-pushed the chao-15 branch from 2de9cfa to 244fb33 Compare December 3, 2020 14:33

knative-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 3, 2020

zhanggbj approved these changes Dec 3, 2020

View reviewed changes

knative-prow-robot assigned zhanggbj Dec 3, 2020

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

maximilien reviewed Dec 3, 2020

View reviewed changes

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 4, 2020

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 4, 2020

chaozbj added 5 commits December 4, 2020 20:42

add autoscaling list command

85f55db

Signed-off-by: chaozbj <[email protected]>

update README.adoc

f2a2f9a

Signed-off-by: chaozbj <[email protected]>

print default config values and add e2e test case

9c4c0e6

Signed-off-by: chaozbj <[email protected]>

update go.mod same as the knative/client v0.17

2500ebe

Signed-off-by: chaozbj <[email protected]>

rebase from master and delete release.sh

386b9a3

Signed-off-by: chaozbj <[email protected]>

chaozbj force-pushed the chao-15 branch from 244fb33 to 386b9a3 Compare December 4, 2020 12:52

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 4, 2020

revert maximilien/kn-source-pkg to 0.4.8

4e4a2b4

Signed-off-by: chaozbj <[email protected]>

chaozbj closed this Dec 4, 2020

chaozbj mentioned this pull request Dec 4, 2020

add autoscaling list command #29

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add autoscaling list command #17

add autoscaling list command #17

chaozbj commented Nov 12, 2020

zhanggbj left a comment

maximilien left a comment

maximilien Nov 18, 2020

maximilien Nov 18, 2020

chaozbj Nov 20, 2020

chaozbj Nov 20, 2020

zhanggbj Nov 20, 2020 •

edited

Loading

zhanggbj commented Nov 20, 2020

zhanggbj commented Nov 27, 2020

chaozbj commented Nov 30, 2020 •

edited

Loading

zhanggbj commented Nov 30, 2020

chaozbj commented Dec 3, 2020 •

edited

Loading

knative-prow-robot commented Dec 3, 2020

zhanggbj commented Dec 3, 2020

zhanggbj commented Dec 3, 2020

maximilien left a comment

navidshaikh commented Dec 4, 2020

zhanggbj commented Dec 4, 2020

zhanggbj commented Dec 4, 2020

chaozbj commented Dec 4, 2020

chaozbj commented Dec 4, 2020

navidshaikh commented Dec 4, 2020 •

edited

Loading

knative-prow-robot commented Dec 4, 2020

chaozbj commented Dec 4, 2020

add autoscaling list command #17

add autoscaling list command #17

Conversation

chaozbj commented Nov 12, 2020

zhanggbj left a comment

Choose a reason for hiding this comment

maximilien left a comment

Choose a reason for hiding this comment

maximilien Nov 18, 2020

Choose a reason for hiding this comment

maximilien Nov 18, 2020

Choose a reason for hiding this comment

chaozbj Nov 20, 2020

Choose a reason for hiding this comment

chaozbj Nov 20, 2020

Choose a reason for hiding this comment

zhanggbj Nov 20, 2020 • edited Loading

Choose a reason for hiding this comment

zhanggbj commented Nov 20, 2020

zhanggbj commented Nov 27, 2020

chaozbj commented Nov 30, 2020 • edited Loading

zhanggbj commented Nov 30, 2020

chaozbj commented Dec 3, 2020 • edited Loading

knative-prow-robot commented Dec 3, 2020

zhanggbj commented Dec 3, 2020

zhanggbj commented Dec 3, 2020

maximilien left a comment

Choose a reason for hiding this comment

navidshaikh commented Dec 4, 2020

zhanggbj commented Dec 4, 2020

zhanggbj commented Dec 4, 2020

chaozbj commented Dec 4, 2020

chaozbj commented Dec 4, 2020

navidshaikh commented Dec 4, 2020 • edited Loading

knative-prow-robot commented Dec 4, 2020

chaozbj commented Dec 4, 2020

zhanggbj Nov 20, 2020 •

edited

Loading

chaozbj commented Nov 30, 2020 •

edited

Loading

chaozbj commented Dec 3, 2020 •

edited

Loading

navidshaikh commented Dec 4, 2020 •

edited

Loading