fix: add --set and --set-value-file flag support to delete command and unit tests#10054
Conversation
There was a problem hiding this comment.
Code Review
This pull request enables the --set flag for the skaffold delete command and introduces a unit test to verify the presence of the --set and --dry-run flags. Feedback suggests enhancing the test to verify flag binding and ensuring isolation by overriding package-level variables. Additionally, it is recommended to enable the --set-value-file flag for the delete command to maintain consistency with other commands that support manifest overrides.
0291ecc to
4ffd77a
Compare
…nd unit tests Signed-off-by: Bogdan Nazarenko <bogdan.nazarenko@outlook.com>
4ffd77a to
ef4594e
Compare
|
Seems that the unit test require you to run: ./hack/generate-man.sh |
Signed-off-by: Bogdan Nazarenko <bogdan.nazarenko@outlook.com>
|
@Darien-Lin the failed integration tests are not related to these changes. Looks like they are pre-existing failures |
Description
This PR adds support for the
--setand--set-value-fileflag to theskaffold deletecommand, allowing users to override templated manifest fields with key-value pairs during deletion operations.Fixes #10053
Changes made:
cmd/skaffold/app/cmd/flags.goline 764 to include "delete" in theDefinedOnlist for the "set" and "set-value-file" flagscmd/skaffold/app/cmd/delete_test.gowith unit tests to verify that the delete command accepts--set,--set-value-fileand--dry-runflags--setand--set-value-fileflag for the delete commandUser facing changes
Users can now use the
--setand--set-value-fileflags withskaffold deleteto override templated manifest fields:Before:
skaffold deletedid not accept--setor--set-value-fileflagsAfter:
skaffold delete --set key=value --set set-value-file=fileis now supportedThis allows for more flexible deletion operations when working with templated manifests.
Testing
TestNewCmdDeleteindelete_test.gothat verifies both--set,--set-value-fileand--dry-runflags are available