Support test plugins with `context-aware-cli-for-plugins` feature-flag enabled #3276

anujc25 · 2022-09-07T07:00:55Z

What this PR does / why we need it

This PR adds support for installing test plugin along with plugin installation in local mode with tanzu test fetch --local command.

The change also implements FetchTest interface for local artifacts and throws error when using the same with oci and http artifacts.
This change also updates Plugin Test pipeline to not rely on old implementation and uses new context-aware-cli-for-plugins: true to deploy and run test plugins.

Which issue(s) this PR fixes

Fixes #1164

Describe testing done for PR

Build the cli and plugins locally. Generates artifacts under artifacts/darwin/amd64/cli/ directory
Use tanzu plugin install all --local artifacts/darwin/amd64/cli/ to install plugins along with test plugins
Run tanzu test plugin --help to verify all plugins are available under the command to run tests
Run tanzu test plugin <plugin-name> to run the plugin tests

Release note

Support test plugins with `context-aware-cli-for-plugins` feature-flag enabled

Additional information

Special notes for your reviewer

codecov · 2022-09-07T07:46:04Z

Codecov Report

Merging #3276 (89f80fb) into main (ed3014a) will decrease coverage by 0.37%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #3276      +/-   ##
==========================================
- Coverage   54.21%   53.83%   -0.38%     
==========================================
  Files         107       91      -16     
  Lines       10781    10019     -762     
==========================================
- Hits         5845     5394     -451     
+ Misses       4467     4185     -282     
+ Partials      469      440      -29

Impacted Files	Coverage Δ
addons/controllers/machine_controller.go	`65.65% <0.00%> (-3.04%)`	⬇️
pkg/v1/sdk/capabilities/discovery/tkg/resource.go
pkg/v1/sdk/capabilities/discovery/generate.go
pkg/v1/sdk/capabilities/discovery/discovery.go
pkg/v1/sdk/capabilities/discovery/fake.go
cmd/cli/plugin/pinniped-auth/main.go
.../v1/sdk/capabilities/discovery/tkg/capabilities.go
...kg/v1/sdk/capabilities/discovery/cluster_object.go
...v1/sdk/capabilities/discovery/tkg/cloudprovider.go
pkg/v1/sdk/capabilities/discovery/tkg/fake.go
... and 8 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

saji-pivotal

Great work! I have a few questions though

cli/core/pkg/artifact/local.go

cli/core/pkg/artifact/local_test.go

cli/core/pkg/distribution/artifact.go

cli/core/pkg/pluginmanager/manager.go

chandrareddyp · 2022-09-21T22:09:34Z

I ran below commands, and got output as Plugin tests

❯ tanzu test plugin --help
Plugin tests
❯ tanzu test plugin cluster
Plugin tests

IMO, as an end-user, I would expect more details as output!
this is first time I ran tanzu test, I don't know if its a relevant question!

chandrareddyp · 2022-09-21T22:37:58Z

I ran the below commands to test plugins, but I don't see any output; it might be successful, but I think it would be better if we print output.

❯ tanzu test plugin feature
❯ tanzu test plugin secret
❯ tanzu test plugin package

cli/core/pkg/artifact/local.go

anujc25 · 2022-09-22T21:24:34Z

I ran the below commands to test plugins, but I don't see any output; it might be successful, but I think it would be better if we print output.

I was looking into this and found out that as part of invocation, Tanzu CLI just invokes test plugin binary for a specified plugin when we do tanzu test plugin <plugin-name> and the test plugin is responsible for printing the status on the stdout.

As part of the plugin initialization that builder plugin does, we bootstrap test/main.go file for the plugin so that we can show some output of the test run. It turns out that plugins like package, secret, feature has removed that piece of the code from its test plugin codebase and hence we are not seeing anything in the output.

I feel that is a plugin specific code and Tanzu CLI might not be able to show any output in that case if plugin author decide not to print anything on the terminal because the test plugin is not getting used.

anujc25 · 2022-09-22T21:58:50Z

I ran below commands, and got output as Plugin tests
❯ tanzu test plugin --help
Plugin tests
❯ tanzu test plugin cluster
Plugin tests
IMO, as an end-user, I would expect more details as output! this is first time I ran tanzu test, I don't know if its a relevant question!

I verified and tanzu test plugin --help is shown as expected from this PR branch.

Regarding tanzu test plugin cluster when not installed showing helptext. I have filled this bug (#3473) that we should address separately.

marckhouzam

Thanks @anujc25 for the nice improvement.
It is hard for me to understand the subtleties since I don't know what was the old behaviour, so my questions may not make much sense, but I'll try anyway.

The (very small) README of the test plugin mentions that to install all the tests plugins, we should use tanzu test fetch. In your PR description you used tanzu plugin install all instead. What do you prefer users to do between the two approaches? And if we use tanzu test fetch, would it allow us to avoid installing the tests plugins when doing a tanzu plugin install?

cmd/cli/plugin-admin/test/main.go

cli/runtime/apis/cli/v1alpha1/catalog_types.go

marckhouzam · 2022-09-23T02:13:33Z

Sorry, by previous comment should have used tanzu test fetch (and not tanzu plugin fetch). I have corrected it.

…enabled Signed-off-by: Anuj Chaudhari <anujc@vmware.com>

Signed-off-by: Anuj Chaudhari <anujc@vmware.com>

anujc25 · 2022-09-23T06:52:32Z

The (very small) README of the test plugin mentions that to install all the tests plugins, we should use tanzu test fetch. In your PR description you used tanzu plugin install all instead. What do you prefer users to do between the two approaches? And if we use tanzu test fetch, would it allow us to avoid installing the tests plugins when doing a tanzu plugin install?

Good point. I think we should not install test plugin as part of plugin installation but just install the test plugin when we do tanzu test fetch --local. I am not sure about the previous behavior either but this makes more sense. I have updated the implementation accordingly. cc @marckhouzam

vuil

lgtm, thanks!

marckhouzam

LGTM

rajathagasthya

Just minor comments. No blockers.

rajathagasthya · 2022-09-26T02:53:21Z

cli/core/pkg/artifact/http.go

+
+// FetchTest returns test artifact
+func (g *HTTPArtifact) FetchTest() ([]byte, error) {
+	return nil, errors.New("fetching test plugin from HTTP source is not yet supported")


nit: just use fmt.Errorf and avoid an external dependency.

I feel it would be better to use errors package consistently across all the files. Most of the other places we are using errors.New, errors.Wrap, errors.Wrapf to handle the error.

Now that fmt.Errorf supports wrapping with %w, I don't feel pkg/errors is necessary anymore.

Thanks for the feedback. This is good to know.

But if you don't mind, I would prefer to do that change as a separate PR and not hold this PR for that.

cli/core/pkg/artifact/local.go

cli/core/pkg/pluginmanager/manager.go

Signed-off-by: Anuj Chaudhari <anujc@vmware.com>

anujc25 requested a review from a team as a code owner September 7, 2022 07:00

vmwclabot added the cla-not-required label Sep 7, 2022

anujc25 force-pushed the support-test-plugin-with-context-aware-cli-v2 branch 6 times, most recently from 26d36af to 3db991d Compare September 7, 2022 22:38

vuil added the do-not-merge/wip Still work in progress label Sep 13, 2022

anujc25 force-pushed the support-test-plugin-with-context-aware-cli-v2 branch 7 times, most recently from daaf6a5 to 3a76513 Compare September 20, 2022 04:47

anujc25 changed the title ~~[Draft] Support test plugins with context-aware-cli-for-plugins feature-flag enabled~~ Support test plugins with context-aware-cli-for-plugins feature-flag enabled Sep 20, 2022

anujc25 added area/core-cli and removed do-not-merge/wip Still work in progress labels Sep 20, 2022

saji-pivotal suggested changes Sep 21, 2022

View reviewed changes

chandrareddyp reviewed Sep 21, 2022

View reviewed changes

cli/core/pkg/artifact/local.go Show resolved Hide resolved

anujc25 force-pushed the support-test-plugin-with-context-aware-cli-v2 branch from 3a76513 to aed7a58 Compare September 22, 2022 20:34

saji-pivotal approved these changes Sep 22, 2022

View reviewed changes

marckhouzam reviewed Sep 23, 2022

View reviewed changes

cmd/cli/plugin-admin/test/main.go Outdated Show resolved Hide resolved

cmd/cli/plugin-admin/test/main.go Outdated Show resolved Hide resolved

cli/runtime/apis/cli/v1alpha1/catalog_types.go Outdated Show resolved Hide resolved

anujc25 added 2 commits September 22, 2022 22:49

Support test plugins with context-aware-cli-for-plugins feature-flag …

f58b908

…enabled Signed-off-by: Anuj Chaudhari <anujc@vmware.com>

Remove TestPluginInstallationPath

c2e5862

Signed-off-by: Anuj Chaudhari <anujc@vmware.com>

anujc25 force-pushed the support-test-plugin-with-context-aware-cli-v2 branch from aed7a58 to dcb934b Compare September 23, 2022 06:49

vuil approved these changes Sep 25, 2022

View reviewed changes

vuil added ok-to-merge PRs should be labelled with this before merging area/dx Related to developer experience labels Sep 25, 2022

marckhouzam approved these changes Sep 25, 2022

View reviewed changes

rajathagasthya reviewed Sep 26, 2022

View reviewed changes

Only install test plugin with tanzu test fetch command

89f80fb

Signed-off-by: Anuj Chaudhari <anujc@vmware.com>

anujc25 force-pushed the support-test-plugin-with-context-aware-cli-v2 branch from dcb934b to 89f80fb Compare September 27, 2022 19:29

anujc25 merged commit e77877a into vmware-tanzu:main Sep 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support test plugins with `context-aware-cli-for-plugins` feature-flag enabled #3276

Support test plugins with `context-aware-cli-for-plugins` feature-flag enabled #3276

anujc25 commented Sep 7, 2022 •

edited

Loading

codecov bot commented Sep 7, 2022 •

edited

Loading

saji-pivotal left a comment

chandrareddyp commented Sep 21, 2022 •

edited

Loading

chandrareddyp commented Sep 21, 2022

anujc25 commented Sep 22, 2022

anujc25 commented Sep 22, 2022 •

edited

Loading

marckhouzam left a comment •

edited

Loading

marckhouzam commented Sep 23, 2022

anujc25 commented Sep 23, 2022

vuil left a comment

marckhouzam left a comment

rajathagasthya left a comment

rajathagasthya Sep 26, 2022

anujc25 Sep 27, 2022

rajathagasthya Sep 28, 2022

anujc25 Sep 28, 2022

Support test plugins with context-aware-cli-for-plugins feature-flag enabled #3276

Support test plugins with context-aware-cli-for-plugins feature-flag enabled #3276

Conversation

anujc25 commented Sep 7, 2022 • edited Loading

What this PR does / why we need it

Which issue(s) this PR fixes

Describe testing done for PR

Release note

Additional information

Special notes for your reviewer

codecov bot commented Sep 7, 2022 • edited Loading

Codecov Report

saji-pivotal left a comment

Choose a reason for hiding this comment

chandrareddyp commented Sep 21, 2022 • edited Loading

chandrareddyp commented Sep 21, 2022

anujc25 commented Sep 22, 2022

anujc25 commented Sep 22, 2022 • edited Loading

marckhouzam left a comment • edited Loading

Choose a reason for hiding this comment

marckhouzam commented Sep 23, 2022

anujc25 commented Sep 23, 2022

vuil left a comment

Choose a reason for hiding this comment

marckhouzam left a comment

Choose a reason for hiding this comment

rajathagasthya left a comment

Choose a reason for hiding this comment

rajathagasthya Sep 26, 2022

Choose a reason for hiding this comment

anujc25 Sep 27, 2022

Choose a reason for hiding this comment

rajathagasthya Sep 28, 2022

Choose a reason for hiding this comment

anujc25 Sep 28, 2022

Choose a reason for hiding this comment

Support test plugins with `context-aware-cli-for-plugins` feature-flag enabled #3276

Support test plugins with `context-aware-cli-for-plugins` feature-flag enabled #3276

anujc25 commented Sep 7, 2022 •

edited

Loading

codecov bot commented Sep 7, 2022 •

edited

Loading

chandrareddyp commented Sep 21, 2022 •

edited

Loading

anujc25 commented Sep 22, 2022 •

edited

Loading

marckhouzam left a comment •

edited

Loading