search

knative-extensions / kn-plugin-admin

Kn plugin for managing a Kubernetes based Knative installation.
Apache License 2.0
7 stars 18 forks source link

add autoscaling list command #17

Closed chaozbj closed 3 years ago

chaozbj commented 3 years ago

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
zhanggbj commented 3 years ago

/hold wait for #16 to merge first.

zhanggbj commented 3 years ago

/unhold

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

chaozbj commented 3 years ago

@zhanggbj @maximilien Changes as below:

  1. 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.

  2. 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.

  3. Updated unit test codes for new changes.

  4. Added e2e test case for autoscaling list command.

  5. 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!

zhanggbj commented 3 years ago

@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 https://github.com/knative-sandbox/kn-plugin-admin/pull/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 commented 3 years ago

@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 commented 3 years ago

[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](https://github.com/knative-sandbox/kn-plugin-admin/blob/master/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 commented 3 years ago

/lgtm Look nice and merging! Thanks @chaozbj!

zhanggbj commented 3 years ago

Prow tests are failing due to test-infra setting in progress which is covered by https://github.com/knative-sandbox/kn-plugin-admin/pull/24

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

/test all

zhanggbj commented 3 years ago

/retest

zhanggbj commented 3 years ago

/lgtm cancel

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

chaozbj commented 3 years ago

/retest

chaozbj commented 3 years ago

/retest

navidshaikh commented 3 years ago

@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

knative-prow-robot commented 3 years ago

@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 4e4a2b402ca31f572a7a25a882a141298bcd9989 link /test pull-knative-sandbox-kn-plugin-admin-build-tests
pull-knative-sandbox-kn-plugin-admin-integration-tests 4e4a2b402ca31f572a7a25a882a141298bcd9989 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](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
chaozbj commented 3 years ago

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