Skip to content
This repository has been archived by the owner on Oct 12, 2023. It is now read-only.

Raise error for duplicate application names #69

Merged
merged 3 commits into from
Dec 18, 2020
Merged

Raise error for duplicate application names #69

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a55611c
Raise error for duplicate application names
jopit Dec 9, 2020
a9b4354
Updates based on review comments
jopit Dec 10, 2020
261a102
Update pkg/controllers/applicationset_controller.go
jopit Dec 17, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 16 additions & 0 deletions pkg/controllers/applicationset_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package controllers
import (
"context"
"encoding/json"
"errors"
"fmt"
"reflect"
"sort"
Expand Down Expand Up @@ -78,6 +79,10 @@ func (r *ApplicationSetReconciler) Reconcile(req ctrl.Request) (ctrl.Result, err
if err != nil {
return ctrl.Result{}, err
}
err = validateDesiredApplications(desiredApplications)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can be part of the generateApplications, if there is a duplicate name the generate will fail with an error. what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's cleaner if the validation is done separately from the generation, particularly in light of Jonathan's comments above about the reconcile being retried when there is an error. If validation is separate, an error from generateApplications will still result in the reconcile being retried, while a duplicate application name will not.

Also, if there is a problem generating one of the applications, generateApplications currently will return both an error and a slice containing the valid applications (I'm not sure why). I think weaving the return of an error for duplicate names into this logic would be unnecessarily complicated.

if err != nil {
return ctrl.Result{}, fmt.Errorf("Reconciling ApplicationSet %s: %w", applicationSetInfo.Name, err)
jopit marked this conversation as resolved.
Show resolved Hide resolved
}

if r.Policy.Update() {
err = r.createOrUpdateInCluster(ctx, applicationSetInfo, desiredApplications)
Expand Down Expand Up @@ -190,6 +195,17 @@ func addInvalidGeneratorNames(names map[string]bool, applicationSetInfo *argopro
}
}

func validateDesiredApplications(desiredApplications []argov1alpha1.Application) error {
for i1, app1 := range desiredApplications {
for i2, app2 := range desiredApplications {
if i1 != i2 && app1.Name == app2.Name {
return errors.New("application set contains applications with duplicate names")
jopit marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
return nil
}

func (r *ApplicationSetReconciler) GetRelevantGenerators(requestedGenerator *argoprojiov1alpha1.ApplicationSetGenerator) []generators.Generator {
var res []generators.Generator

Expand Down
60 changes: 60 additions & 0 deletions pkg/controllers/applicationset_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1109,3 +1109,63 @@ func TestCheckInvalidGenerators(t *testing.T) {
hook.Reset()
}
}

func TestValidateDesiredApplicationsDuplicateApplicationNames(t *testing.T) {

scheme := runtime.NewScheme()
err := argoprojiov1alpha1.AddToScheme(scheme)
assert.Nil(t, err)
err = argov1alpha1.AddToScheme(scheme)
assert.Nil(t, err)

for _, c := range []struct {
testName string
desiredApps []argov1alpha1.Application
isError bool
}{
{
testName: "has no duplicates",
desiredApps: []argov1alpha1.Application{
{
ObjectMeta: metav1.ObjectMeta{
Name: "app1",
},
},
{
ObjectMeta: metav1.ObjectMeta{
Name: "app2",
},
},
},
isError: false,
},
{
testName: "has duplicates",
desiredApps: []argov1alpha1.Application{
{
ObjectMeta: metav1.ObjectMeta{
Name: "app1",
},
},
{
ObjectMeta: metav1.ObjectMeta{
Name: "app2",
},
},
{
ObjectMeta: metav1.ObjectMeta{
Name: "app1",
},
},
},
isError: true,
},
} {
err := validateDesiredApplications(c.desiredApps)
if c.isError {
assert.Error(t, err, c.testName)
} else {
assert.NoError(t, err, c.testName)
}
}
}