knative-extensions / kn-plugin-admin

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

Avoid to print runtime error if kubectl context is not set #40

Closed chaozbj closed 3 years ago

chaozbj commented 3 years ago

This PR is used to address issue #32, following kn/client, we will delay creating kubenets client interface till subcommand running.

Fixes #32

codecov[bot] commented 3 years ago

Codecov Report

Merging #40 (5a5ed94) into master (f4b8a49) will increase coverage by 0.49%. The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #40      +/-   ##
==========================================
+ Coverage   78.76%   79.26%   +0.49%     
==========================================
  Files          20       20              
  Lines        1003     1032      +29     
==========================================
+ Hits          790      818      +28     
+ Misses        156      155       -1     
- Partials       57       59       +2     
Impacted Files Coverage Δ
core/root.go 60.00% <ø> (ø)
pkg/types.go 22.00% <18.18%> (+2.00%) :arrow_up:
pkg/command/profiling/profiling.go 81.85% <84.61%> (-0.50%) :arrow_down:
pkg/command/autoscaling/list.go 92.85% <100.00%> (+0.26%) :arrow_up:
pkg/command/autoscaling/update.go 70.27% <100.00%> (+0.82%) :arrow_up:
pkg/command/domain/list.go 85.18% <100.00%> (+1.85%) :arrow_up:
pkg/command/domain/set.go 96.49% <100.00%> (+0.19%) :arrow_up:
pkg/command/domain/unset.go 94.11% <100.00%> (+0.56%) :arrow_up:
pkg/command/registry/add.go 92.94% <100.00%> (+0.25%) :arrow_up:
pkg/command/registry/list.go 87.93% <100.00%> (+0.65%) :arrow_up:
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update f4b8a49...7f8c8ce. Read the comment docs.

chaozbj commented 3 years ago

@rhuss @zhanggbj @lanceliuu Please help me to review, thanks a lot! @lanceliuu In types.go file, I used the

return err

to replace your codes when fail to check installation method:

fmt.Println("failed to get installation method:", err)
os.Exit(1)

Because the os.Exit(1) will break some UT. I tried to keep them and find some ways to make UT pass, but failed. If the installation method is not necessary for every subcommand, I would like to recommend to delay getting it till the subcommand really needs it.

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 /assign @maximilien

Looks great! Thanks for fixing the issue and especially improving test coverage. Merging it!