Use kctrl command tree by default in package plugin #3207
Conversation
Codecov Report
@@ Coverage Diff @@
## main #3207 +/- ##
==========================================
- Coverage 46.58% 45.75% -0.83%
==========================================
Files 400 425 +25
Lines 39722 41278 +1556
==========================================
+ Hits 18504 18888 +384
- Misses 19519 20681 +1162
- Partials 1699 1709 +10
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| By("disable kctrl command tree") | ||
| command := exec.NewCommand( | ||
| exec.WithCommand("tanzu"), | ||
| exec.WithArgs("config", "set", "features.package.kctrl-package-command-tree", "false"), |
There was a problem hiding this comment.
Until when this flag going to stay false on test? (Is it part of the plan to make downstream integration tests eventually pass with kctrl feature enabled as well or if there is another way to for e2e testing kctrl-enabled package/secret plugin functionality?)
There was a problem hiding this comment.
I think modifying existing test libs which package_plugin_integration_test uses would not work, as will break earlier versions tests.
You might want to add your own set of test libs for kctrl feature being enabled.
There was a problem hiding this comment.
Until when this flag going to stay false on test?
I am guessing until the old behaviour is deprecated/removed and we no longer need these tests.
Is it part of the plan to make downstream integration tests eventually pass with kctrl feature enabled as well or if there is another way to for e2e testing kctrl-enabled package
kctrl has it's own set of e2e tests upstream and there are some tests that make sure that the differences in ux are taken care of. I felt that we would just be duplicating those upstream tests here and hence didn't add any new tests, but please let me know what y'all think.
There was a problem hiding this comment.
Thanks for the clarification @praveenrewar. As far as I know, TF tests would be needed to get a feature enabled & merged in Tanzu Framework. Also test-infra e2e tests need to get modified to point to the e2e tests corresponding to kctrl feature after 1.7 release. I let @vijaykatam and @shyaamsn decide on this.
There was a problem hiding this comment.
@praveenrewar As @maralavi mentioned we cant enable kctrl in tanzu package plugin without tests even though they are tested upstream. We need to test tanzu package plugin in tanzu-framework and downstream.
There are two options:
- Fix these tests in tanzu-framework so that they can run.
- Make kctrl tests work in tanzu-framework and downstream
There was a problem hiding this comment.
Thanks @maralavi and @shyaamsn for sharing your thoughts.
Fix these tests in tanzu-framework so that they can run.
Sure, I think I can add more tests over here which will work cover the kctrl command tree enabled scenario (the only concern is that I have already mentioned, but I don't mind it)
Also test-infra e2e tests need to get modified to point to the e2e tests corresponding to kctrl feature after 1.7 release
Which tests are we talking about here, and do we need to make this change as part of the same PR? (Still trying to make my way through the codebase 😅 )
Make kctrl tests work in tanzu-framework and downstream
Are you referring to the upstream kctrl tests?
There was a problem hiding this comment.
Which tests are we talking about here
Hi @praveenrewar , the tests are those in this file which tests all commands end to end, that you are just disabling with your PR.
do we need to make this change as part of the same PR? (Still trying to make my way through the codebase 😅 )
Yes unfortunately Tanzu Framework code base policy is that everything should be tested properly as part of the same PR:)
There was a problem hiding this comment.
Ah, okay. That makes sense.
|
@praveenrewar Could you please take a look at the failing tests ? Also please resolve the conflicts. Another question on tests: So in package plugin tests now we test both kctrl enabled and disabled is it ? |
@shyaamsn I found an issue in kctrl upstream and was working on resolving that. I will create a separate PR to bump kctrl version. Meanwhile I have marked it as a draft PR.
Yeah, I thought it would be good to have tests for both. But do let me know if it's an overkill (because we are no longer making any changes to the legacy code for package plugin). |
No having both tests is great. Do we have to enable something to make both the tests run or by default both the tests run ? By default having both tests run is wonderful. |
By default both the tests run :) |
|
@shyaamsn @maralavi It seems that bumping kctrl (which results in bumping a bunch of other dependencies) results in this error. I think it's probably due to a method being updated in the newer versions. I will take a good look at it tomorrow but if y'all have any idea about how to fix this then please let me know :) |
|
Cluster Generation A/B Results: |
|
Thanks for the changes @praveenrewar |
What this PR does / why we need it
Use kctrl command tree by default in package plugin.
Which issue(s) this PR fixes
Fixes #3761
Describe testing done for PR
Release note