Skip to content

Commit

Permalink
Fix the way action set name was being configured (#2430)
Browse files Browse the repository at this point in the history
* Fix the way action set name was configred

`kanctl` was able to configure the actionset name using the flag called
`action-set`.
This has two problems
1. The better flag to give actionset name would be `--name` instead of
`--action-set`. Becuase there is another flag called `action`, so it
gets confusing between `--action-set` and `--action`.
2. Even though user would expect that if they have provided `--name`
the actionset would be named exactly that. But the problem was we
were using `generateName` in code that resulted in a string being
added in the name.

* Fix golint

* Fix typo

---------

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
  • Loading branch information
viveksinghggits and mergify[bot] authored Oct 26, 2023
1 parent f3021dc commit 7c924c3
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 10 deletions.
15 changes: 8 additions & 7 deletions pkg/kanctl/actionset.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/util/rand"
"k8s.io/client-go/kubernetes"
"sigs.k8s.io/yaml"

Expand All @@ -40,7 +41,7 @@ import (

const (
actionFlagName = "action"
actionSetFlagName = "action-set"
actionSetFlagName = "name"
blueprintFlagName = "blueprint"
configMapsFlagName = "config-maps"
deploymentFlagName = "deployment"
Expand Down Expand Up @@ -190,7 +191,7 @@ func newActionSet(params *PerformParams) (*crv1alpha1.ActionSet, error) {

return &crv1alpha1.ActionSet{
ObjectMeta: metav1.ObjectMeta{
GenerateName: name,
Name: name,
},
Spec: &crv1alpha1.ActionSetSpec{
Actions: actions,
Expand Down Expand Up @@ -254,7 +255,7 @@ func ChildActionSet(parent *crv1alpha1.ActionSet, params *PerformParams) (*crv1a

return &crv1alpha1.ActionSet{
ObjectMeta: metav1.ObjectMeta{
GenerateName: name,
Name: name,
},
Spec: &crv1alpha1.ActionSetSpec{
Actions: actions,
Expand Down Expand Up @@ -741,19 +742,19 @@ func max(x, y int) int {

func generateActionSetName(p *PerformParams) (string, error) {
if p.ActionSetName != "" {
return fmt.Sprintf("%s-", p.ActionSetName), nil
return p.ActionSetName, nil
}

if p.ActionName != "" {
if p.ParentName != "" {
return fmt.Sprintf("%s-%s-", p.ActionName, p.ParentName), nil
return fmt.Sprintf("%s-%s-%s", p.ActionName, p.ParentName, rand.String(5)), nil
}

return fmt.Sprintf("%s-", p.ActionName), nil
return fmt.Sprintf("%s-%s", p.ActionName, rand.String(5)), nil
}

if p.ParentName != "" {
return fmt.Sprintf("%s-", p.ParentName), nil
return fmt.Sprintf("%s-%s", p.ParentName, rand.String(5)), nil
}

return "", errMissingFieldActionName
Expand Down
12 changes: 9 additions & 3 deletions pkg/kanctl/actionset_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,8 @@ func (k *KanctlTestSuite) TestGenerateActionSetName(c *C) {
{actionName: "my-action", actionSetName: "", parentName: "", expected: "my-action-"},
{actionName: "my-action", actionSetName: "", parentName: "parent", expected: "my-action-parent-"},
{actionName: "", actionSetName: "", parentName: "parent", expected: "parent-"},
{actionName: "my-action", actionSetName: "my-override", parentName: "parent", expected: "my-override-"},
{actionName: "", actionSetName: "my-override", parentName: "", expected: "my-override-"},
{actionName: "my-action", actionSetName: "my-override", parentName: "parent", expected: "my-override"},
{actionName: "", actionSetName: "my-override", parentName: "", expected: "my-override"},
}

for _, tc := range testCases {
Expand All @@ -102,6 +102,12 @@ func (k *KanctlTestSuite) TestGenerateActionSetName(c *C) {

actual, err := generateActionSetName(params)
c.Assert(err, DeepEquals, tc.expectedErr)
c.Assert(actual, DeepEquals, tc.expected)
if tc.actionSetName != "" || tc.expected == "" {
// if --name is provided we just use that we dont derive name
c.Assert(actual, DeepEquals, tc.expected)
} else {
// random 5 chars are added at the end if name is derived by us
c.Assert(actual[0:len(actual)-5], DeepEquals, tc.expected)
}
}
}

0 comments on commit 7c924c3

Please sign in to comment.