Feat: add more detailed output for `vela addon status` #3876

charlie0129 · 2022-05-12T15:27:48Z

Description of your changes

Fixes #3860

vela addon status now shows more detailed info.

Screenshots

Before

After

If an addon is installed, and it also exists in the registry:
- non-verbose output
- verbose output
If an addon is not installed, but it does exist in the registry:
- non-verbose output
- verbose output
If an addon is installed, but it does not exist in the registry:
- non-verbose output
- verbose output
If an addon is not installed, nor does it exist in the registry:
- non-verbose output (since it does not connect to network when -v/--verbose is off, it is not possible to know whether this addon is in registry, so, only disabled is shown)
- verbose output (now we check the registry to know that this addon does not exist, an error will be returned. It is handled by upstream error handlers, and return code will not be 0, indicating an error:

I have:

Read and followed KubeVela's contribution process.
Related Docs updated properly. In a new feature or configuration option, an update to the documentation is necessary.
Run make reviewable to ensure this PR is ready for review.
Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

Relevant unit tests have been added, which will test the four situations above.

Unit tests for helper functions have been created as well.

Special notes for your reviewer

Also prettify output Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>

Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>

if a local addon has been installed, the AVAILABLE-VERSIONS column of the registry one will not truncate. this commit fixes that. Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>

Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>

Limit Description length when using `addon list` Use simpler method to get addon info Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>

Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>

…here Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>

Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>

…egistry Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>

Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>

codecov · 2022-05-12T15:48:35Z

Codecov Report

Merging #3876 (149c2ab) into master (20f1d54) will increase coverage by 0.09%.
The diff coverage is 81.11%.

@@            Coverage Diff             @@
##           master    #3876      +/-   ##
==========================================
+ Coverage   64.12%   64.22%   +0.09%     
==========================================
  Files         312      316       +4     
  Lines       29692    30453     +761     
==========================================
+ Hits        19039    19557     +518     
- Misses       8194     8369     +175     
- Partials     2459     2527      +68

Flag	Coverage Δ
apiserver-unittests	`35.05% <18.88%> (-0.17%)`	⬇️
core-unittests	`55.89% <81.11%> (-0.77%)`	⬇️
e2e-multicluster-test	`27.04% <0.00%> (+0.42%)`	⬆️
e2e-rollout-tests	`23.19% <ø> (-0.38%)`	⬇️
e2etests	`30.27% <ø> (-1.00%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/addon/error.go	`16.66% <ø> (ø)`
pkg/addon/helper.go	`69.27% <80.00%> (+7.20%)`	⬆️
pkg/addon/versioned_registry.go	`68.47% <100.00%> (+6.40%)`	⬆️
pkg/cmd/factory.go	`63.15% <0.00%> (-36.85%)`	⬇️
pkg/apiserver/rest/usecase/converter.go	`56.75% <0.00%> (-34.91%)`	⬇️
pkg/resourcekeeper/statekeep.go	`46.80% <0.00%> (-27.11%)`	⬇️
pkg/utils/k8s.go	`74.46% <0.00%> (-14.11%)`	⬇️
...tepdefinition/workflowstepdefinition_controller.go	`69.14% <0.00%> (-7.45%)`	⬇️
pkg/apiserver/sync/store.go	`55.00% <0.00%> (-4.00%)`	⬇️
pkg/workflow/providers/multicluster/deploy.go	`79.77% <0.00%> (-2.95%)`	⬇️
... and 42 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 20f1d54...149c2ab. Read the comment docs.

Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>

pkg/addon/helper.go

… to get addon status Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>

Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>

wonderflow · 2022-05-14T06:57:41Z

If an addon is not installed, nor does it exist in the registry:

Why not show the addon is not existed in this case?

For other cases, this PR is really really great! 👍

wonderflow · 2022-05-14T07:02:01Z

pkg/addon/helper.go

+				merge(wholePackage)
+			}
+		} else {
+			meta, err := r.ListAddonMeta()


will it use cache here? I'm not sure if the performance will be very bad here. \cc @wangyikewxgm

Maybe, we can downgrade the experience to the old mode if the registry is not versioned.

it is only called when

vela cli

verbose is on (default is off)

non-versioned registry is used

So the user must explicitly say "hey I need detailed info", otherwise this code won't be executed. It think the performance here is not a big deal (with local registries, no noticeable delay when using cli).

pkg/addon/versioned_registry.go

references/cli/addon.go

wonderflow

generally LGTM, ping @wangyikewxgm for review, mainly about the performance.

We should keep the vela addon status command response quickly.

charlie0129 · 2022-05-14T07:32:10Z

Why not show the addon is not existed in this case?

Great suggestion, which will make the output more informative to the user.

Will implement it!

… the registry Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>

…erflow Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>

wangyikewxgm · 2022-05-16T02:38:32Z

@wonderflow The main costs is fetching whole addon package. Because user cannot get knowledge of the parameter info about this addon by vela cli before, so I design this feature. Please note that the GetAddonStatus in helper will be used in apiserver and cli, it must be quickly @charlie0129 . So I suggest add --verbose flag in vela addon status, if true cli will fetch whole package and show the parameter info, otherwise won't. The apiserver calls this func needn't fetch the whole package.

charlie0129 · 2022-05-16T03:35:41Z

@wonderflow The main costs is fetching whole addon package. Because user cannot get knowledge of the parameter info about this addon by vela cli before, so I design this feature. Please note that the GetAddonStatus in helper will be used in apiserver and cli, it must be quickly @charlie0129 . So I suggest add --verbose flag in vela addon status, if true cli will fetch whole package and show the parameter info, otherwise won't. The apiserver calls this func needn't fetch the whole package.

Yes, that is why I worried about the performance issue at first. But now GetAddonStatus doesn't get the whole package. It just checks current parameters from secrets, which (I think) is relatively fast. The actual process of getting the whole package is in generateAddonInfo, which is called only when vela addon status is executed.

wangyikewxgm · 2022-05-16T04:08:12Z

@wonderflow The main costs is fetching whole addon package. Because user cannot get knowledge of the parameter info about this addon by vela cli before, so I design this feature. Please note that the GetAddonStatus in helper will be used in apiserver and cli, it must be quickly @charlie0129 . So I suggest add --verbose flag in vela addon status, if true cli will fetch whole package and show the parameter info, otherwise won't. The apiserver calls this func needn't fetch the whole package.

Yes, that is why I worried about the performance issue at first. But now GetAddonStatus doesn't get the whole package. It just checks current parameters from secrets, which (I think) is relatively fast. The actual process of getting the whole package is in generateAddonInfo, which is called only when vela addon status is executed.

Please be aware that if the network is too slow to get the whole addon package from registry, user should still check the addon status quickly, this is why I suggest show detail info only when verbose flag is true.

charlie0129 · 2022-05-16T05:10:25Z

So I suggest add --verbose flag in vela addon status

Good idea. Will be implemented!

Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>

…addon is not installed, but it does exist in the registry Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>

Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>

wonderflow

great job!

Somefive

Fantastic work!

wangyikewxgm

LGTM

charlie0129 added 14 commits May 11, 2022 19:15

Feat: show available versions and dependencies in addon status

4945854

Also prettify output Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>

Feat: show addon dependencies in addon status

5b597a0

Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>

Style: adjust addon status output

65ab376

Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>

Style: remove spaces in addon status output

475de5c

Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>

Fix: truncate string if available versions too long

86b58a3

if a local addon has been installed, the AVAILABLE-VERSIONS column of the registry one will not truncate. this commit fixes that. Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>

Perf: avoid unnecessary network requests to improve performance

b3b505e

Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>

Feat: do not display remote info for local addons

daf6f4b

Limit Description length when using `addon list` Use simpler method to get addon info Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>

Test: add tests for cli/addon limitStringLength, generateParameterString

d18dfa2

Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>

Fix: fix output being empty if the searched addon does not exist anyw…

e52d693

…here Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>

Test: add tests for generateAddonInfo

f0d2545

Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>

Test: add tests for generateAddonInfo and FindWholeAddonPackagesFromR…

e479369

…egistry Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>

Style: format code

572fe25

Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>

Test: correct unnecessarily focused tests

ee240b6

Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>

Style: remove unused code blocks

59d48c2

Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>

charlie0129 requested review from Somefive, zzxwill, barnettZQG, wonderflow and leejanee as code owners May 12, 2022 15:27

Refactor: remove unnecessary output to stdout

acfc9c8

Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>

charlie0129 added 3 commits May 13, 2022 00:27

Test: remove added registry after tests has been completed

66452dc

Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>

Test: new RegistryDataStore every test, to avoid panics

cb76b87

Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>

Test: fix failing tests due to incorrect registry name

084f316

Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>

zzxwill reviewed May 12, 2022

View reviewed changes

pkg/addon/helper.go Outdated Show resolved Hide resolved

charlie0129 added 2 commits May 13, 2022 16:17

Refactor: move logic to get addon package out of where it is supposed…

d3de49b

… to get addon status Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>

Test: add test cases for secret-getting in TestGetAddonStatus

8b296d6

Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>

wonderflow reviewed May 14, 2022

View reviewed changes

pkg/addon/versioned_registry.go Outdated Show resolved Hide resolved

wonderflow reviewed May 14, 2022

View reviewed changes

references/cli/addon.go Outdated Show resolved Hide resolved

wonderflow approved these changes May 14, 2022

View reviewed changes

charlie0129 added 2 commits May 14, 2022 23:32

Feat: return error if an addon is not installed, nor does it exist in…

e3398c7

… the registry Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>

Refactor: rename GetAddonWholePackage to GetDetailedAddon as per wond…

3e7cd76

…erflow Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>

charlie0129 added 8 commits May 18, 2022 18:30

Feat: require --verbose option to show details in vela addon status

01ffdde

Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>

Feat: require --verbose option to show details in vela addon status

54dd5bc

Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>

Test: update tests to make sure --verbose is on when testing

7683698

Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>

chore: run make reviewable

176bdfd

Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>

Test: fix configmap being already existed

923a476

Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>

Test: update exsiting tests to prevent already existing error

5e2674a

Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>

Fix: fix output show addon does not exist when verbose is off and an …

d62d003

…addon is not installed, but it does exist in the registry Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>

Style: fix golangci-lint issues

149c2ab

Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>

wonderflow approved these changes May 19, 2022

View reviewed changes

Somefive approved these changes May 19, 2022

View reviewed changes

wangyikewxgm approved these changes May 19, 2022

View reviewed changes

wangyikewxgm merged commit eb5f416 into kubevela:master May 19, 2022

charlie0129 mentioned this pull request Jul 27, 2022

Self-nominate @charlie0129 as approver kubevela/community#58

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: add more detailed output for `vela addon status` #3876

Feat: add more detailed output for `vela addon status` #3876

charlie0129 commented May 12, 2022 •

edited

Loading

codecov bot commented May 12, 2022 •

edited

Loading

wonderflow commented May 14, 2022 •

edited

Loading

wonderflow May 14, 2022

charlie0129 May 19, 2022 •

edited

Loading

wonderflow left a comment

charlie0129 commented May 14, 2022

wangyikewxgm commented May 16, 2022

charlie0129 commented May 16, 2022

wangyikewxgm commented May 16, 2022

charlie0129 commented May 16, 2022

wonderflow left a comment

Somefive left a comment

wangyikewxgm left a comment

Feat: add more detailed output for vela addon status #3876

Feat: add more detailed output for vela addon status #3876

Conversation

charlie0129 commented May 12, 2022 • edited Loading

Description of your changes

Screenshots

Before

After

How has this code been tested

Special notes for your reviewer

codecov bot commented May 12, 2022 • edited Loading

Codecov Report

wonderflow commented May 14, 2022 • edited Loading

wonderflow May 14, 2022

Choose a reason for hiding this comment

charlie0129 May 19, 2022 • edited Loading

Choose a reason for hiding this comment

wonderflow left a comment

Choose a reason for hiding this comment

charlie0129 commented May 14, 2022

wangyikewxgm commented May 16, 2022

charlie0129 commented May 16, 2022

wangyikewxgm commented May 16, 2022

charlie0129 commented May 16, 2022

wonderflow left a comment

Choose a reason for hiding this comment

Somefive left a comment

Choose a reason for hiding this comment

wangyikewxgm left a comment

Choose a reason for hiding this comment

Feat: add more detailed output for `vela addon status` #3876

Feat: add more detailed output for `vela addon status` #3876

charlie0129 commented May 12, 2022 •

edited

Loading

codecov bot commented May 12, 2022 •

edited

Loading

wonderflow commented May 14, 2022 •

edited

Loading

charlie0129 May 19, 2022 •

edited

Loading