Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: create analysisrun cmd using template generated name #1471

Merged
merged 2 commits into from
Sep 10, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions pkg/kubectl-argo-rollouts/cmd/create/create.go
Original file line number Diff line number Diff line change
Expand Up @@ -274,11 +274,11 @@ func NewCmdCreateAnalysisRun(o *options.ArgoRolloutsOptions) *cobra.Command {
}
}

objName, notFound, err := unstructured.NestedString(obj.Object, "metadata", "name")
objName, found, err := unstructured.NestedString(obj.Object, "metadata", "name")
if err != nil {
return err
}
if !notFound {
if found {
Comment on lines -277 to +281
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops!

templateName = objName
}

Expand All @@ -292,6 +292,10 @@ func NewCmdCreateAnalysisRun(o *options.ArgoRolloutsOptions) *cobra.Command {
}
ns := o.Namespace()

if name == "" && generateName == "-" {
return fmt.Errorf("name is invalid")
}

obj, err = analysisutil.NewAnalysisRunFromUnstructured(obj, templateArgs, name, generateName, ns)
if err != nil {
return err
Expand Down
15 changes: 15 additions & 0 deletions pkg/kubectl-argo-rollouts/cmd/create/create_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,21 @@ func TestCreateAnalysisRunName(t *testing.T) {
assert.Empty(t, stderr)
}

func TestCreateAnalysisRunNameNotSpecified(t *testing.T) {
tf, o := options.NewFakeArgoRolloutsOptions()
defer tf.Cleanup()

cmd := NewCmdCreateAnalysisRun(o)
cmd.PersistentPreRunE = o.PersistentPreRunE
cmd.SetArgs([]string{"--from-file", "testdata/analysis-template-no-name.yaml", "-a", "foo=bar"})
err := cmd.Execute()
assert.EqualError(t, err, "name is invalid")
stdout := o.Out.(*bytes.Buffer).String()
stderr := o.ErrOut.(*bytes.Buffer).String()
assert.Empty(t, stdout)
assert.Equal(t, "Error: name is invalid\n", stderr)
}

func TestCreateAnalysisRunWithInstanceID(t *testing.T) {
tf, o := options.NewFakeArgoRolloutsOptions()
defer tf.Cleanup()
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
kind: AnalysisTemplate
apiVersion: argoproj.io/v1alpha1
metadata:
spec:
args:
- name: foo
metrics:
- name: pass
interval: 5s
failureLimit: 1
provider:
job:
spec:
template:
spec:
containers:
- name: sleep
image: alpine:3.8
command: [sh, -c]
args: [exit 0]
restartPolicy: Never
backoffLimit: 0
16 changes: 14 additions & 2 deletions utils/analysis/helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import (
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime"
patchtypes "k8s.io/apimachinery/pkg/types"
"k8s.io/apimachinery/pkg/util/validation/field"

argoprojclient "github.com/argoproj/argo-rollouts/pkg/client/clientset/versioned/typed/rollouts/v1alpha1"
)
Expand Down Expand Up @@ -373,7 +372,17 @@ func NewAnalysisRunFromUnstructured(obj *unstructured.Unstructured, templateArgs
return nil, err
}

// Remove resourceVersion if exists
_, found, err := unstructured.NestedString(obj.Object, "metadata", "resourceVersion")
Comment on lines +375 to +376
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this fix imply we never had --from in-cluster-analysis-template working properly?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, the unit test failed to catch that because of the fake client.

if err != nil {
return nil, err
}
if found {
unstructured.RemoveNestedField(obj.Object, "metadata", "resourceVersion")
}

// Set args
newArgVals := []interface{}{}
for i := 0; i < len(newArgs); i++ {
var newArgInterface map[string]interface{}
newArgBytes, err := json.Marshal(newArgs[i])
Expand All @@ -384,7 +393,10 @@ func NewAnalysisRunFromUnstructured(obj *unstructured.Unstructured, templateArgs
if err != nil {
return nil, err
}
err = unstructured.SetNestedMap(obj.Object, newArgInterface, field.NewPath("spec", "args").Index(i).String())
newArgVals = append(newArgVals, newArgInterface)
}
if len(newArgVals) > 0 {
err = unstructured.SetNestedSlice(obj.Object, newArgVals, "spec", "args")
if err != nil {
return nil, err
}
Expand Down
57 changes: 57 additions & 0 deletions utils/analysis/helpers_test.go
Original file line number Diff line number Diff line change
@@ -1,20 +1,23 @@
package analysis

import (
"encoding/json"
"errors"
"fmt"
"testing"

log "github.com/sirupsen/logrus"
"github.com/stretchr/testify/assert"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
kunstructured "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"
kubetesting "k8s.io/client-go/testing"
"k8s.io/utils/pointer"

"github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1"
"github.com/argoproj/argo-rollouts/pkg/client/clientset/versioned/fake"
"github.com/argoproj/argo-rollouts/utils/unstructured"
)

func TestIsWorst(t *testing.T) {
Expand Down Expand Up @@ -630,6 +633,60 @@ func TestMergeArgs(t *testing.T) {
}
}

func TestNewAnalysisRunFromUnstructured(t *testing.T) {
template := v1alpha1.AnalysisTemplate{
ObjectMeta: metav1.ObjectMeta{
Name: "foo",
Namespace: metav1.NamespaceDefault,
ResourceVersion: "12345",
},
Spec: v1alpha1.AnalysisTemplateSpec{
Metrics: []v1alpha1.Metric{
{
Name: "success-rate",
},
},
Args: []v1alpha1.Argument{
{
Name: "my-arg-1",
},
{
Name: "my-arg-2",
},
},
},
}
args := []v1alpha1.Argument{
{
Name: "my-arg-1",
Value: pointer.StringPtr("my-val-1"),
},
{
Name: "my-arg-2",
Value: pointer.StringPtr("my-val-2"),
},
}

jsonStr, err := json.Marshal(template)
assert.NoError(t, err)
obj, err := unstructured.StrToUnstructured(string(jsonStr))
assert.NoError(t, err)

obj, err = NewAnalysisRunFromUnstructured(obj, args, "foo-run", "foo-run-generate-", "my-ns")
assert.NoError(t, err)
_, found, err := kunstructured.NestedString(obj.Object, "metadata", "resourceVersion")
assert.NoError(t, err)
assert.False(t, found)
arArgs, _, err := kunstructured.NestedSlice(obj.Object, "spec", "args")
assert.NoError(t, err)
assert.Equal(t, len(args), len(arArgs))

for i, arg := range arArgs {
argnv := arg.(map[string]interface{})
assert.Equal(t, *args[i].Value, argnv["value"])
}
}

//TODO(dthomson) remove this test in v0.9.0
func TestNewAnalysisRunFromTemplate(t *testing.T) {
template := v1alpha1.AnalysisTemplate{
Expand Down