Package plugin and kctrl integration #1864

praveenrewar · 2022-03-21T12:25:32Z

What this PR does / why we need it

Use kctrl to add a command tree to package plugin.
Update package plugin README
Modify package plugin integration tests

Which issue(s) this PR fixes

Describe testing done for PR

tkg package plugin integration tests
Manual testing on workload cluster

Release note

TBD

PR Checklist

Squash the commits into one or a small number of logical commits
Use good commit messages
Ensure PR contains terms all contributors can understand and links all contributors can access

Additional information

Special notes for your reviewer

github-actions · 2022-03-21T12:26:14Z

Hi @praveenrewar! And thank you for opening your first Pull Request. Someone will review it soon. Thank you for committing to making Tanzu Framework better.

maralavi · 2022-03-23T04:34:48Z

cmd/cli/plugin/package/main.go

-	}
-	return format
-}
+func nonExitingMain(p *plugin.Plugin) error {


Why the name nonExitingMain? (Cause it looks like to me it is a non-blocking function while the name says something else, the name a bit confuses me)

The main() function uses os.Exit() to exit when an error is returned and therefore wouldn't run the defer functions. We use a function which simply returns an error instead of using os.Exit(), hence the name nonExitingMain.
But, I see how it could be a bit confusing, let me know if you have any suggestions for an alternative :)

It is an odd name. Can we call the function what it is doing? Something like kctrlInvoke?

I am okay with kctrlInvoke, however that's not the only thing this function is doing, it's also executing the cmd.

cmd/cli/plugin/package/ui.go

maralavi · 2022-03-23T05:07:58Z

cmd/cli/plugin/package/ui.go

+}
+
+func (adapterUI *AdapterUI) PrintLinef(pattern string, args ...interface{}) {
+	if strings.Contains(pattern, "Target cluster") {


Could you please add a comment on what this function is meant to do?

Also the name of the function is so generic, yet it excludes some specific string like "Target cluster", and that is confusing

Could you please add a comment on what this function is meant to do?

Sure. Will do.

Also the name of the function is so generic, yet it excludes some specific string like "Target cluster", and that is confusing

The idea is to override the kctrl ui using an adapter ui, so I can't use another name here, but the exclusion of certain strings simply based on a specific pattern bothers me as well. I will see how I can improve it.

Agree with @maralavi that the check is confusing. Could we link the issue that is resolving this in the PR?

Ideally this should be done using the tty flag, but there is an issue with the tty flag where in it is always set to true unless the output is being piped. The issue exists in all the carvel tools and we will be fixing it soon.

Once the tty issue is fixed, we should be able to remove the PrintLinef "override".

github-actions · 2022-03-30T22:14:23Z

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/1864/20220330221421/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

github-actions · 2022-03-31T12:36:38Z

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/1864/20220331123636/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

github-actions · 2022-03-31T18:04:02Z

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/1864/20220331180401/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

github-actions · 2022-04-01T17:40:19Z

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/1864/20220401174017/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

github-actions · 2022-04-04T06:04:09Z

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/1864/20220404060407/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

github-actions · 2022-04-04T18:38:32Z

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/1864/20220404183830/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

pkg/v1/tkg/test/tkgpackageclient/package_plugin_integration_test.go

cmd/cli/plugin/package/README.md

maralavi · 2022-04-04T21:16:21Z

cmd/cli/plugin/package/README.md

-   Added package repository 'standard-repo'
+   >>> tanzu package repository add standard-repo --url projects-stg.registry.vmware.com/tkg/test-packages/standard-repo:v1.0.0 -n test-ns
+   Waiting for package repository to be added
+   4:04:00PM: packagerepository/standard-repo (packaging.carvel.dev/v1alpha1) namespace: test-ns: Reconciling


is this 4:04:00PM part of the output?

This deviates from ISO 8601 format used by tanzu CLI https://github.com/vmware-tanzu/tanzu-framework/blob/main/docs/cli/style_guide.md#time-format

i think in this case it's a good exception to the rule -- we want to provide "non-distracting" indication of time passing as output is being added to the screen, user is not interested in full timestamp on every line as it is just noise.

Would be good to document that is a divergence from rest of plugins with the reasoning.

Sure. Please let me know the best place to document this.

cmd/cli/plugin/package/test/lib/package_plugin.go

github-actions · 2022-04-07T20:17:51Z

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/1864/20220407201749/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

github-actions · 2022-04-11T04:34:20Z

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/1864/20220411043418/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

Bump kapp-controller to v0.34.0 and run go mod tidy

Attach kctrl package command tree to package plugin

The ui adapter is used to make kctrl package command tree consistent with package plugin

Modify tkgpackageclient integration tests according to changes introduced by kctrl

maralavi · 2022-05-09T01:12:09Z

Thanks for the changes @praveenrewar . In Order to make sure no regression has happened, I built from this branch and tested some of the package plugin commands with kctrl integration. The followings are some comments for the commands I tested:
- I noticed namespace creation flag (create-namespace) has been removed from package installed create. That would take care of creating the namespace for the user in case not already existing. Was that intended and part of a design choice? (Per kctrl design)

Was removal of -v shorthand for --version intended? (I didn't see it in the design doc)
Output message for package installed update doesn’t seem complete, I do not see any log message for the actual update, and only see one liner Get: (needs fix)

tanzu package installed update test-carvel-pkg -n test-ns
Getting package install for 'test-carvel-pkg'

As a reference, in the existing package plugin (not using kctrl), the output was as follows:

artifacts/darwin/amd64/cli/core/v0.21.0-dev/tanzu-core-darwin_amd64 package installed update test-carvel-pkg -n test-ns --version 3.0.0-rc.1 -p pkg.test.carvel.dev
 Updating installed package 'test-carvel-pkg'
 Getting package install for 'test-carvel-pkg'
 Getting package metadata for 'pkg.test.carvel.dev'
Updated installed package 'test-carvel-pkg' in namespace 'test-ns'

And when not providing the version, it was as follows:

artifacts/darwin/amd64/cli/core/v0.21.0-dev/tanzu-core-darwin_amd64 package installed update test-carvel-pkg -n test-ns
Error: please provide --version and/or --values-file for updating the installed package
Error: exit status 1

Error message for package installed update for a non-existing package, when neither package name or package version is not provided, is not helpful enough. What does the error message Expected to find at least one version, but did not (details: a... would mean in the following? (needs fix)

tanzu package installed update test-carvel-pkg --version 3.0.0-rc.1 -n test-ns --install
Getting package install for 'test-carvel-pkg'
Updating package install for 'test-carvel-pkg'
Waiting for PackageInstall reconciliation for 'test-carvel-pkg'
3:46:46PM: packageinstall/test-carvel-pkg (packaging.carvel.dev/v1alpha1) namespace: test-ns: ReconcileFailed
Error: packageinstall/test-carvel-pkg (packaging.carvel.dev/v1alpha1) namespace: test-ns: Reconciling: . Reconcile failed: Expected to find at least one version, but did not (details: a...

For the same package installed update, when I provided both package name and version, I got the following output though I expected the update to succeed regardless. Has the previous update gone through partly? If yes, that is a bug as we want operations to be idempotent. (needs fix)

tanzu package installed update test-carvel-pkg -p pkg.test.carvel.dev  --version 3.0.0-rc.1 -n test-ns --install
Getting package install for 'test-carvel-pkg'
Error: Installed package 'test-carvel-pkg' is already associated with package ''

Could the not found errors get changed to include the namespace they are looking for to help the user a bit more? For example in the following case, could the error message get changed to Error: packageinstalls.packaging.carvel.dev "test-carvel-pkg" not found in namespace "default" (minor improvement suggestion)

tanzu package installed get test-carvel-pkg
Error: packageinstalls.packaging.carvel.dev "test-carvel-pkg" not found

package installed get with --values-file-output seem to not be working: (needs fix)

 tanzu package installed get test-carvel-pkg -n test-ns --values-file-output output_file
Error: Expected 1 values reference, found 0
Error: exit status 1

package installed get with values seem to not be working: (needs fix)

 tanzu package installed get test-carvel-pkg --namespace test-ns  --values
Error: Expected 1 values reference, found 0
Error: exit status 1

Question: Global flags such as column and --yes don’t work for all commands, like install doesn’t need --column or get & list do not need --yes, yet these are shown as global flags for all. Is this an intended behavior? (minor improvement suggestion)
package installed list example for list for all namespaces is incorrectly using -A not A (minor improvemnet suggestion) :

List installed packages in all namespaces
tanzu package installed list A

for package available get, for the case when the --values-schema flag is provided, and the user has not provided version in the package name, the error message can be more clarifying by telling the user the syntax in which they should use package_name/package_version, so that they know they should provide something like pkg.test.carvel.dev/2.0.0 (minor improvement suggestion):

 tanzu package available get pkg.test.carvel.dev.2.0.0  --values-schema -n test-ns
Error: Package version is required when --values-schema flag is declared
Error: exit status 1

cmd/cli/plugin/package/README.md

praveenrewar · 2022-05-09T15:00:08Z

@maralavi Thanks for looking into the PR. I will reply to all the questions/suggestions in some time, but most of them are design changes that we did intentionally and some of them are good suggestions that I would like to take up with another PR.

praveenrewar · 2022-05-09T19:26:31Z

I noticed namespace creation flag (create-namespace) has been removed from package installed create. That would take care of creating the namespace for the user in case not already existing. Was that intended and part of a design choice?

Yep, this was intended. I am thinking of adding these changes to the release notes if that will be helpful.

Was removal of -v shorthand for --version intended?

I honestly don't remember if this was intended or not. If not, I will definitely make this change in the next release of kctrl.

Output message for package installed update doesn’t seem complete, I do not see any log message for the actual update, and only see one liner Get:

I guess this happening because there's no update to make. We could definitely add an error message here or maybe a hint.

Error message for package installed update for a non-existing package, when either package name or package version is not provided, is not helpful enough. What does the error message Expected to find at least one version, but did not (details: a... would mean in the following?

The error message is actually surfaced from kapp-controller, I will look into how we can tweak it a bit.

For the same package installed update, when I provided both package name and version, I got the following output though I expected the update to succeed regardless.

Yeah, this seems like a bug to me. This would definitely need resolution before the current PR is merged.

Could the not found errors get changed to include the namespace they are looking for to help the user a bit more? For example in the following case, could the error message get changed to Error: packageinstalls.packaging.carvel.dev "test-carvel-pkg" not found in namespace "default"

Seems like a reasonable ask. I will update it.

package installed get with --values-file-output seem to not be working:
tanzu package installed get test-carvel-pkg -n test-ns --values-file-output output_file
Error: Expected 1 values reference, found 0
Error: exit status 1

The error message suggests that the installed package is not using any values file. We can maybe try to improve the error message a bit.

package installed get with values seem to not be working:
Same as above.

Question: Global flags such as column and --yes`` don’t work for all commands, like install doesn’t need --columnor get & list do not need--yes```, yet these are shown as global flags for all. Is this an intended behavior?

Yep, this is also intended.

minor: package installed list example for list for all namespaces is incorrectly using -A not A:

Good catch. I will update it.

I will try to address the bug in this PR itself by making changes in kctrl and bumping the commit hash and for the rest of suggestions I will try to make the changes in the same PR as much as possible, but I feel that a separate PR should also be fine. Let me know your thoughts :)

maralavi · 2022-05-09T20:07:46Z

Thanks @praveenrewar, The fixes don't need to be in this PR. As long as it be individual issues in the kctrl repo for the need-to-be-fixed ones so that the issues can be tracked to eventually get fixed, it should all be good.

Manually run commands on a management cluster and update package README

github-actions · 2022-05-10T12:28:15Z

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/1864/20220510122813/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

praveenrewar · 2022-05-10T14:11:06Z

Hey @maralavi, I have created the following issues in kctrl for the things that you mentioned as needs to be fixed:

Issue to remove the --install flag in package installed update. This should fix the issues that you faced with package installed update
Issue to fix the logging/message with package installed update when there's nothing to update.
Issue for some of the minor suggestions.

package installed get with --values-file-output seem to not be working: (needs fix)

This is an intended behaviour if there was no values file used to install the package. Currently, when using the --values-file flag, it creates an empty file and we wanted to change that experience. (~~This difference is not present in the design doc as it was introduced around the same time as the design doc was created~~ I was thinking about something else, this feature was there before the doc was created)

package installed get with values seem to not be working: (needs fix)

Same as above.

maralavi · 2022-05-10T21:39:11Z

Hey @maralavi, I have created the following issues in kctrl for the things that you mentioned as needs to be fixed:

Issue to remove the --install flag in package installed update. This should fix the issues that you faced with package installed update

Issue to fix the logging/message with package installed update when there's nothing to update.

Issue for some of the minor suggestions.

package installed get with --values-file-output seem to not be working: (needs fix)

This is an intended behaviour if there was no values file used to install the package. Currently, when using the --values-file flag, it creates an empty file and we wanted to change that experience. (This difference is not present in the design doc as it was introduced around the same time as the design doc was created)

package installed get with values seem to not be working: (needs fix)

Same as above.

Thanks @praveenrewar for filing the issues.

About update there is another case when the update goes half through when no package name is provided and then it fails and then the second time it complains that this package install is already associated with empty package name ''. This seems like idemptency is broken in update. As ideally we want to be able to repeat the operation as many times without ending up in a situation like this. Could I please ask for a ticket for this one as well and post the link here? Thank you:)

About the --values-file-output and the --values, could the error message get changed to be a bit more user friendly? As the user might still use these flags to see what happens even when not having provided values file to begin with and the current output is not informative. Could we have ticket for making these outputs more informative?

I also had another comment about values-schema flag (the last one in #1864 (comment)), that I'd appreciate you take a look, and file ticket if needed?

Changes LGTM, but still need to be reviewed & approved by Vijay before getting merged.

addons/go.mod

praveenrewar · 2022-05-11T04:48:20Z

@maralavi Thank you so much!
I have created this issue for package installed update and added more items to the existing issue for updating error messages.

vijaykatam · 2022-05-11T16:36:23Z

Added do-not-merge/hold because @shyaamsn asked me to review, I will review next week. Meanwhile could we ensure that the functionality exposed by https://github.com/vmware-tanzu/tanzu-framework/blob/main/cmd/cli/plugin/package/package_available_get.go#L47-L51 has been ported over to the new CLI.

vijaykatam

We need to get agreement from PM on changes to existing flags to see if this constitutes a major version bump and if that is acceptable. To be clear, I am not opposed to the changes here but we need agreement before getting this into a release.

vijaykatam · 2022-05-12T03:03:21Z

cmd/cli/plugin/package/README.md

+  -h, --help                        help for package
+      --kube-api-burst int          Set Kubernetes API client burst limit (default 1000)
+      --kube-api-qps float32        Set Kubernetes API client QPS limit (default 1000)
+      --kubeconfig string           Path to the kubeconfig file ($TANZU_KUBECONFIG)


Where are these env vars($TANZU_*) being defined? Is this a new tanzu CLI convention that I am probably not aware of?

@vijaykatam , Please let me know whom should i contact to take this thread forward?

I pinged on a thread.

I too am wondering how the env vars are being used.

On a related note there is an ongoing effort to introduce the concept of tanzu context
(#2200)
to consolidate the cli endpoints access (including that via kubeconfigs). It may be worth consider aligning the UX with it.
cc @giri-varma

These environment variables are defined in kctrl (the name is changed using the binary name provided through the adapter).

Please log an issue to address as a followup.

(minor): Also the param --kubeconfig-yaml is odd. Why not use just use --kubeconfig like kubectl

Please log an issue to address as a followup.

Sure. However I am not very clear on what should be covered as part of it. Do we need to remove this convention?

Also the param --kubeconfig-yaml is odd. Why not use just use --kubeconfig like kubectl

We do have --kubeconfigto provide the path to the kubeconfig file. --kubeconfig-yaml can be used to provide the content as yaml.

vijaykatam · 2022-05-12T03:09:23Z

cmd/cli/plugin/package/README.md

-    CATEGORY:                         [ingress]
-
-    Created default values file at /home/contour-default-values.yaml
+   >>> tanzu package available get contour.tanzu.vmware.com/1.18.2+vmware.1-tkg.1 --default-values-file-output contour-default-values.yaml


We need to make the doc team aware of changed params for all these commands. Please log an issue downstream for the doc team. cc @pspinrad @stuclem

@vijaykatam , who will log the issue?

I am hoping your team can log an issue and work with doc team to review the changes that will land in https://docs.vmware.com/en/VMware-Tanzu-Kubernetes-Grid/index.html.

vijaykatam · 2022-05-16T20:42:49Z

cmd/cli/plugin/package/README.md

-   Added package repository 'standard-repo'
+   >>> tanzu package repository add standard-repo --url projects-stg.registry.vmware.com/tkg/test-packages/standard-repo:v1.0.0 -n test-ns
+   Waiting for package repository to be added
+   4:04:00PM: packagerepository/standard-repo (packaging.carvel.dev/v1alpha1) namespace: test-ns: Reconciling


This deviates from ISO 8601 format used by tanzu CLI https://github.com/vmware-tanzu/tanzu-framework/blob/main/docs/cli/style_guide.md#time-format

vijaykatam · 2022-05-16T20:45:57Z

cmd/cli/plugin/package/README.md

-    NAME                      VERSION                RELEASED-AT  NAMESPACE
-    contour.tanzu.vmware.com  1.15.1+vmware.1-tkg.1               test-ns
+    NAMESPACE  NAME                      VERSION                RELEASED-AT
+    test-ns    contour.tanzu.vmware.com  1.15.1+vmware.1-tkg.1  0001-01-01 00:00:00 +0000 UTC


Released-at field is empty for most packages. We should not be displaying a datetime that doesn't make sense.

Hmm, this does make sense. Would it be okay to make this enhancement in a future PR after having some brainstorming on this?

Yes, I am fine but lets get an issue logged.

Created an issue for this.
carvel-dev/kapp-controller#700

vijaykatam · 2022-05-16T20:50:06Z

cmd/cli/plugin/package/main.go

-	}
-	return format
-}
+func nonExitingMain(p *plugin.Plugin) error {


It is an odd name. Can we call the function what it is doing? Something like kctrlInvoke?

vijaykatam · 2022-05-16T20:54:23Z

cmd/cli/plugin/package/ui.go

+// Copyright 2022 VMware, Inc. All Rights Reserved.
+// SPDX-License-Identifier: Apache-2.0
+
+package main


why is this in main?

I am pretty neutral on keeping this in main or a separate nested package.
I guess I had kept it in main to avoid more imports.

vijaykatam · 2022-05-16T20:58:12Z

cmd/cli/plugin/package/ui.go

+	tableCount   int
+}
+
+// PrintLinef overrides go-cli-ui/ui.PrintLinef


This seems like something to upstream to go-cli-ui/ui.PrintLinef. Override is an odd term because go has no override so I would rename the function to something like PrintLinefWithSkipPattern and update doc.

Override is an odd term because go has no override

Yeah, this might be a bit confusing. I just used the term because we are trying to achieve something similar here.

I would rename the function to something like PrintLinefWithSkipPattern and update doc.

PrinLinef is used in kctrl to print lines, but in case of tanzu cli we don't want PrintLinef to print the header that mentions the cluster ip, hence I used the adapter to override the behaviour of PrintLinef here.

vijaykatam · 2022-05-16T20:58:36Z

cmd/cli/plugin/package/ui.go

+	adapterUI.outputFormat = outputFormat
+}
+
+// PrintTable overrides go-cli-ui/ui.PrintTable


Same comment on override and func name.

Similar to PrintLinef, PrintTable is also used to print table in kctrl, but since the output needs to be a bit different in Tanzu CLI and there should also be options to select output format (like json), this method was added to "override" the behaviour of PrintLinef.

vijaykatam · 2022-05-16T20:58:47Z

cmd/cli/plugin/package/ui.go

+
+// PrintTable overrides go-cli-ui/ui.PrintTable
+// It accepts a table and renders it based on the output format
+//nolint:gocritic // Cannot change the function signature as it is defined in go-cli-ui


Please remove nolint

nolint is used avoid the linting error caused due to the usage of values (and not pointers) for the argument.

Lets fix it then.

Similar to PrintLinef, PrintTable is also used to print table in kctrl, but since the output needs to be a bit different in Tanzu CLI and there should also be options to select output format (like json), this method was added to "override" the behaviour of PrintLinef.

The function signature exists in go-cli-ui and changing it over there would make it backward incompatible, requiring changes in all the tools that are currently using it.

vijaykatam · 2022-05-16T20:59:27Z

cmd/cli/plugin/package/ui.go

+			c.PreRun = func(_ *cobra.Command, args []string) { adapterUI.SetOutputFormat(output) }
+		}
+	}
+}


Seems like this file could use a unit test

I think I didn't add a unit test since there were already integration tests running which verify this functionality, but I guess it wouldn't hurt to add a unit test.

Added tests for the adapter ui.

vijaykatam · 2022-05-16T21:03:41Z

cmd/cli/plugin/package/README.md

+      --column strings              Filter to show only given columns
+      --debug                       Include debug output
+  -h, --help                        help for package
+      --kube-api-burst int          Set Kubernetes API client burst limit (default 1000)


Defaults are super high. What is the reasoning for using these as opposed to defaults set by tools like kubectl or controller-runtime?

These have been carried over over from kapp. In case of kctrl/tanzu cli I think that it shouldn't matter much (except for the new status command that have been introduced in the latest release). We can definitely try to figure out an optimum number for these.

vijaykatam

No blocking concerns from me. Lets get all issues logged and linked here. Needs a release note and approval from PM team prior to merging.

vijaykatam · 2022-05-23T16:07:16Z

cmd/cli/plugin/package/README.md

+  -h, --help                        help for package
+      --kube-api-burst int          Set Kubernetes API client burst limit (default 1000)
+      --kube-api-qps float32        Set Kubernetes API client QPS limit (default 1000)
+      --kubeconfig string           Path to the kubeconfig file ($TANZU_KUBECONFIG)


Please log an issue to address as a followup.

(minor): Also the param --kubeconfig-yaml is odd. Why not use just use --kubeconfig like kubectl

vijaykatam · 2022-05-23T16:07:55Z

cmd/cli/plugin/package/README.md

-   Added package repository 'standard-repo'
+   >>> tanzu package repository add standard-repo --url projects-stg.registry.vmware.com/tkg/test-packages/standard-repo:v1.0.0 -n test-ns
+   Waiting for package repository to be added
+   4:04:00PM: packagerepository/standard-repo (packaging.carvel.dev/v1alpha1) namespace: test-ns: Reconciling


Would be good to document that is a divergence from rest of plugins with the reasoning.

vijaykatam · 2022-05-23T16:08:27Z

cmd/cli/plugin/package/README.md

-    NAME                      VERSION                RELEASED-AT  NAMESPACE
-    contour.tanzu.vmware.com  1.15.1+vmware.1-tkg.1               test-ns
+    NAMESPACE  NAME                      VERSION                RELEASED-AT
+    test-ns    contour.tanzu.vmware.com  1.15.1+vmware.1-tkg.1  0001-01-01 00:00:00 +0000 UTC


Yes, I am fine but lets get an issue logged.

vijaykatam · 2022-05-23T16:08:59Z

cmd/cli/plugin/package/ui.go

+
+// PrintTable overrides go-cli-ui/ui.PrintTable
+// It accepts a table and renders it based on the output format
+//nolint:gocritic // Cannot change the function signature as it is defined in go-cli-ui


Lets fix it then.

github-actions · 2022-05-25T15:11:41Z

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/1864/20220525151139/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

codecov · 2022-05-25T15:56:32Z

Codecov Report

❗ No coverage uploaded for pull request base (main@2ebea9c). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1864   +/-   ##
=======================================
  Coverage        ?   42.66%           
=======================================
  Files           ?      378           
  Lines           ?    38415           
  Branches        ?        0           
=======================================
  Hits            ?    16391           
  Misses          ?    20486           
  Partials        ?     1538

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 2ebea9c...c202ee5. Read the comment docs.

praveenrewar · 2022-05-30T12:27:17Z

@vijaykatam @maralavi Since we have not been able to move forward with a versioning strategy for the breaking changes, we are planning to make this behaviour opt in using tanzu config set. So, by default users will experience the existing behaviour, but they will also have an option to enable the new changes using a feature flag. Once we have a proper versioning strategy we can make this as the default behaviour. Does this sound good to you? (I can raise a quick PR with that change and close this one)

praveenrewar · 2022-05-31T04:19:22Z

Closing this PR with regards to #2522

vmwclabot added the cla-not-required label Mar 21, 2022

maralavi reviewed Mar 23, 2022

View reviewed changes

praveenrewar requested a review from maralavi March 31, 2022 19:42

praveenrewar marked this pull request as ready for review April 4, 2022 18:49

praveenrewar requested review from a team as code owners April 4, 2022 18:49

maralavi reviewed Apr 4, 2022

View reviewed changes

praveenrewar marked this pull request as draft April 7, 2022 06:06

praveenrewar marked this pull request as ready for review April 7, 2022 20:38

praveenrewar added 9 commits May 6, 2022 18:13

Bump kapp-controller to v0.34.0

29b8e16

Bump kapp-controller to v0.34.0 and run go mod tidy

Use kctrl package command tree

925b4f4

Attach kctrl package command tree to package plugin

Add ui adapter for kctrl package command tree

d751326

The ui adapter is used to make kctrl package command tree consistent with package plugin

Remove unused package plugin code

93fefe0

Use ReplaceAll instead of replaceSpaceWithHyphen

8764965

Remove dot import

44f8d5e

Modify tkgpackageclient integration tests

4f58b25

Modify tkgpackageclient integration tests according to changes introduced by kctrl

Add comments to adapter ui and run go mod tidy

14d1453

Add table counter to print only 1 table for -ojson

6e555d4

maralavi reviewed May 9, 2022

View reviewed changes

cmd/cli/plugin/package/README.md Outdated Show resolved Hide resolved

Update README for package management

c3a4950

Manually run commands on a management cluster and update package README

maralavi approved these changes May 10, 2022

View reviewed changes

vijaykatam added the do-not-merge/hold Some fixes necessary, hold for merging label May 10, 2022

maralavi reviewed May 10, 2022

View reviewed changes

addons/go.mod Show resolved Hide resolved

vijaykatam reviewed May 16, 2022

View reviewed changes

vijaykatam reviewed May 23, 2022

View reviewed changes

vijaykatam removed the do-not-merge/hold Some fixes necessary, hold for merging label May 23, 2022

Add test for adapter ui

c202ee5

praveenrewar closed this May 31, 2022

Package plugin and kctrl integration #1864

Package plugin and kctrl integration #1864

Conversation

praveenrewar commented Mar 21, 2022 • edited Loading

What this PR does / why we need it

Which issue(s) this PR fixes

Describe testing done for PR

Release note

PR Checklist

Additional information

Special notes for your reviewer

github-actions bot commented Mar 21, 2022

maralavi Mar 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maralavi Mar 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 30, 2022

github-actions bot commented Mar 31, 2022

github-actions bot commented Mar 31, 2022

github-actions bot commented Apr 1, 2022

github-actions bot commented Apr 4, 2022

github-actions bot commented Apr 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 7, 2022

github-actions bot commented Apr 11, 2022

maralavi commented May 9, 2022 • edited Loading

praveenrewar commented May 9, 2022

praveenrewar commented May 9, 2022

maralavi commented May 9, 2022 • edited Loading

github-actions bot commented May 10, 2022

praveenrewar commented May 10, 2022 • edited Loading

maralavi commented May 10, 2022 • edited Loading

praveenrewar commented May 11, 2022

vijaykatam commented May 11, 2022

vijaykatam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vuil May 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

praveenrewar commented Mar 21, 2022 •

edited

Loading

maralavi Mar 23, 2022 •

edited

Loading

maralavi Mar 23, 2022 •

edited

Loading

maralavi commented May 9, 2022 •

edited

Loading

maralavi commented May 9, 2022 •

edited

Loading

praveenrewar commented May 10, 2022 •

edited

Loading

maralavi commented May 10, 2022 •

edited

Loading

vuil May 17, 2022 •

edited

Loading

praveenrewar May 17, 2022 •

edited

Loading

praveenrewar commented May 30, 2022 •

edited

Loading