Skip to content

Commit

Permalink
Refactor test and MR feedback
Browse files Browse the repository at this point in the history 
Signed-off-by: Abhi Kapoor <[email protected]>
  • Loading branch information
@abhi-kapoor
abhi-kapoor committed Dec 18, 2024
1 parent 0be618b commit 77d2ff8
Show file tree
Hide file tree
Showing 5 changed files with 58 additions and 55 deletions.
4 changes: 2 additions & 2 deletions cmd/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,13 +49,13 @@ var ControllerCmd = &cobra.Command{

// watch app modifications, if necessary
if cfg.MonitorAllApplications {
appWatcher, err := app_watcher.NewApplicationWatcher(ctr)
appWatcher, err := app_watcher.NewApplicationWatcher(ctr, ctx)
if err != nil {
log.Fatal().Err(err).Msg("failed to create watch applications")
}
go appWatcher.Run(ctx, 1)

appSetWatcher, err := app_watcher.NewApplicationSetWatcher(ctr)
appSetWatcher, err := app_watcher.NewApplicationSetWatcher(ctr, ctx)
if err != nil {
log.Fatal().Err(err).Msg("failed to create watch application sets")
}
Expand Down
16 changes: 8 additions & 8 deletions pkg/app_watcher/app_watcher.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ type ApplicationWatcher struct {
// - kubeCfg is the Kubernetes configuration.
// - vcsToArgoMap is the mapping between VCS and Argo applications.
// - cfg is the server configuration.
func NewApplicationWatcher(ctr container.Container) (*ApplicationWatcher, error) {
func NewApplicationWatcher(ctr container.Container, ctx context.Context) (*ApplicationWatcher, error) {
if ctr.KubeClientSet == nil {
return nil, fmt.Errorf("kubeCfg cannot be nil")
}
Expand All @@ -46,7 +46,7 @@ func NewApplicationWatcher(ctr container.Container) (*ApplicationWatcher, error)
vcsToArgoMap: ctr.VcsToArgoMap,
}

appInformer, appLister := ctrl.newApplicationInformerAndLister(time.Second*30, ctr.Config)
appInformer, appLister := ctrl.newApplicationInformerAndLister(time.Second*30, ctr.Config, ctx)

ctrl.appInformer = appInformer
ctrl.appLister = appLister
Expand Down Expand Up @@ -119,8 +119,8 @@ func (ctrl *ApplicationWatcher) onApplicationDeleted(obj interface{}) {
ctrl.vcsToArgoMap.DeleteApp(app)
}

// IsAppNamespaceAllowed is used by both the ApplicationWatcher and the ApplicationSetWatcher
func IsAppNamespaceAllowed(meta *metav1.ObjectMeta, cfg config.ServerConfig) bool {
// isAppNamespaceAllowed is used by both the ApplicationWatcher and the ApplicationSetWatcher
func isAppNamespaceAllowed(meta *metav1.ObjectMeta, cfg config.ServerConfig) bool {
return meta.Namespace == cfg.ArgoCDNamespace || glob.MatchStringInList(cfg.AdditionalAppsNamespaces, meta.Namespace, glob.REGEXP)
}

Expand All @@ -135,7 +135,7 @@ that need to observe the object.
newApplicationInformerAndLister use the data from the informer's cache to provide a read-optimized view of the cache which reduces
the load on the API Server and hides some complexity.
*/
func (ctrl *ApplicationWatcher) newApplicationInformerAndLister(refreshTimeout time.Duration, cfg config.ServerConfig) (cache.SharedIndexInformer, applisters.ApplicationLister) {
func (ctrl *ApplicationWatcher) newApplicationInformerAndLister(refreshTimeout time.Duration, cfg config.ServerConfig, ctx context.Context) (cache.SharedIndexInformer, applisters.ApplicationLister) {

watchNamespace := cfg.ArgoCDNamespace
// If we have at least one additional namespace configured, we need to
Expand All @@ -149,21 +149,21 @@ func (ctrl *ApplicationWatcher) newApplicationInformerAndLister(refreshTimeout t
ListFunc: func(options metav1.ListOptions) (apiruntime.Object, error) {
// We are only interested in apps that exist in namespaces the
// user wants to be enabled.
appList, err := ctrl.applicationClientset.ArgoprojV1alpha1().Applications(watchNamespace).List(context.TODO(), options)
appList, err := ctrl.applicationClientset.ArgoprojV1alpha1().Applications(watchNamespace).List(ctx, options)
if err != nil {
return nil, err
}
newItems := []appv1alpha1.Application{}
for _, app := range appList.Items {
if IsAppNamespaceAllowed(&app.ObjectMeta, cfg) {
if isAppNamespaceAllowed(&app.ObjectMeta, cfg) {
newItems = append(newItems, app)
}
}
appList.Items = newItems
return appList, nil
},
WatchFunc: func(options metav1.ListOptions) (watch.Interface, error) {
return ctrl.applicationClientset.ArgoprojV1alpha1().Applications(watchNamespace).Watch(context.TODO(), options)
return ctrl.applicationClientset.ArgoprojV1alpha1().Applications(watchNamespace).Watch(ctx, options)
},
},
&appv1alpha1.Application{},
Expand Down
76 changes: 38 additions & 38 deletions pkg/app_watcher/app_watcher_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@ import (
)

func initTestObjects(t *testing.T) *ApplicationWatcher {
ctx, cancel := context.WithCancel(context.Background())
defer cancel()

cfg, err := config.New()
cfg.AdditionalAppsNamespaces = []string{"*"}
// Handle the error appropriately, e.g., log it or fail the test
Expand All @@ -41,7 +44,7 @@ func initTestObjects(t *testing.T) *ApplicationWatcher {
vcsToArgoMap: appdir.NewVcsToArgoMap("vcs-username"),
}

appInformer, appLister := ctrl.newApplicationInformerAndLister(time.Second*1, cfg)
appInformer, appLister := ctrl.newApplicationInformerAndLister(time.Second*1, cfg, ctx)
ctrl.appInformer = appInformer
ctrl.appLister = appLister

Expand Down Expand Up @@ -257,45 +260,42 @@ func TestCanProcessApp(t *testing.T) {
}

func TestIsAppNamespaceAllowed(t *testing.T) {

cfg, err := config.New()
// Handle the error appropriately, e.g., log it or fail the test
require.NoError(t, err, "failed to create config")
// set up the fake Application client set and informer.
testApp1 := &v1alpha1.Application{
ObjectMeta: metav1.ObjectMeta{Name: "test-app-1", Namespace: "default-ns"},
Spec: v1alpha1.ApplicationSpec{
Source: &v1alpha1.ApplicationSource{RepoURL: "https://gitlab.com/test/repo.git"},
tests := map[string]struct {
expected bool
cfg config.ServerConfig
meta *metav1.ObjectMeta
}{
"All namespaces for application are allowed": {
expected: true,
cfg: config.ServerConfig{AdditionalAppsNamespaces: []string{"*"}},
meta: &metav1.ObjectMeta{Name: "test-app-1", Namespace: "default-ns"},
},
"Specific namespaces for application are allowed": {
expected: false,
cfg: config.ServerConfig{AdditionalAppsNamespaces: []string{"default", "kube-system"}},
meta: &metav1.ObjectMeta{Name: "test-app-1", Namespace: "default-ns"},
},
"Wildcard namespace for application is allowed": {
expected: true,
cfg: config.ServerConfig{AdditionalAppsNamespaces: []string{"default-*"}},
meta: &metav1.ObjectMeta{Name: "test-app-1", Namespace: "default-ns"},
},
"Invalid characters in namespace for application are not allowed": {
expected: false,
cfg: config.ServerConfig{AdditionalAppsNamespaces: []string{"<default-*", "kube-system"}},
meta: &metav1.ObjectMeta{Name: "test-app-1", Namespace: "default-ns"},
},
"Specific namespace for application set is allowed": {
expected: true,
cfg: config.ServerConfig{AdditionalAppsNamespaces: []string{"<default-*>", "kube-system"}},
meta: &metav1.ObjectMeta{Name: "test-appset-1", Namespace: "kube-system"},
},
}

testAppSet1 := &v1alpha1.ApplicationSet{
ObjectMeta: metav1.ObjectMeta{Name: "test-appset-1", Namespace: "kube-system"},
Spec: v1alpha1.ApplicationSetSpec{},
}

cfg.AdditionalAppsNamespaces = []string{"*"}
if !IsAppNamespaceAllowed(&testApp1.ObjectMeta, cfg) {
t.Error("default-ns namespace should be allowed")
}

cfg.AdditionalAppsNamespaces = []string{"default", "kube-system"}
if IsAppNamespaceAllowed(&testApp1.ObjectMeta, cfg) == true {
t.Error("default-ns namespace should not be allowed")
}

cfg.AdditionalAppsNamespaces = []string{"default-*"}
if IsAppNamespaceAllowed(&testApp1.ObjectMeta, cfg) == false {
t.Error("default-ns namespace should be allowed")
}

cfg.AdditionalAppsNamespaces = []string{"<default-*>", "kube-system"}
if IsAppNamespaceAllowed(&testApp1.ObjectMeta, cfg) {
t.Error("default-ns namespace should not be allowed")
}

cfg.AdditionalAppsNamespaces = []string{"<default-*>", "kube-system"}
if !IsAppNamespaceAllowed(&testAppSet1.ObjectMeta, cfg) {
t.Error("default-ns namespace should not be allowed")
for testName, test := range tests {
t.Run(testName, func(t *testing.T) {
actual := isAppNamespaceAllowed(test.meta, test.cfg)
assert.Equal(t, test.expected, actual)
})
}
}
12 changes: 6 additions & 6 deletions pkg/app_watcher/appset_watcher.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ type ApplicationSetWatcher struct {
}

// NewApplicationSetWatcher creates new instance of ApplicationWatcher.
func NewApplicationSetWatcher(ctr container.Container) (*ApplicationSetWatcher, error) {
func NewApplicationSetWatcher(ctr container.Container, ctx context.Context) (*ApplicationSetWatcher, error) {
if ctr.KubeClientSet == nil {
return nil, fmt.Errorf("kubeCfg cannot be nil")
}
Expand All @@ -39,7 +39,7 @@ func NewApplicationSetWatcher(ctr container.Container) (*ApplicationSetWatcher,
vcsToArgoMap: ctr.VcsToArgoMap,
}

appInformer, appLister := ctrl.newApplicationSetInformerAndLister(time.Second*30, ctr.Config)
appInformer, appLister := ctrl.newApplicationSetInformerAndLister(time.Second*30, ctr.Config, ctx)

ctrl.appInformer = appInformer
ctrl.appLister = appLister
Expand All @@ -63,7 +63,7 @@ func (ctrl *ApplicationSetWatcher) Run(ctx context.Context) {
<-ctx.Done()
}

func (ctrl *ApplicationSetWatcher) newApplicationSetInformerAndLister(refreshTimeout time.Duration, cfg config.ServerConfig) (cache.SharedIndexInformer, applisters.ApplicationSetLister) {
func (ctrl *ApplicationSetWatcher) newApplicationSetInformerAndLister(refreshTimeout time.Duration, cfg config.ServerConfig, ctx context.Context) (cache.SharedIndexInformer, applisters.ApplicationSetLister) {
watchNamespace := cfg.ArgoCDNamespace
// If we have at least one additional namespace configured, we need to
// watch on them all.
Expand All @@ -76,21 +76,21 @@ func (ctrl *ApplicationSetWatcher) newApplicationSetInformerAndLister(refreshTim
ListFunc: func(options metav1.ListOptions) (apiruntime.Object, error) {
// We are only interested in apps that exist in namespaces the
// user wants to be enabled.
appList, err := ctrl.applicationClientset.ArgoprojV1alpha1().ApplicationSets(watchNamespace).List(context.TODO(), options)
appList, err := ctrl.applicationClientset.ArgoprojV1alpha1().ApplicationSets(watchNamespace).List(ctx, options)
if err != nil {
return nil, err
}
newItems := []appv1alpha1.ApplicationSet{}
for _, appSet := range appList.Items {
if IsAppNamespaceAllowed(&appSet.ObjectMeta, cfg) {
if isAppNamespaceAllowed(&appSet.ObjectMeta, cfg) {
newItems = append(newItems, appSet)
}
}
appList.Items = newItems
return appList, nil
},
WatchFunc: func(options metav1.ListOptions) (watch.Interface, error) {
return ctrl.applicationClientset.ArgoprojV1alpha1().ApplicationSets(watchNamespace).Watch(context.TODO(), options)
return ctrl.applicationClientset.ArgoprojV1alpha1().ApplicationSets(watchNamespace).Watch(ctx, options)
},
},
&appv1alpha1.ApplicationSet{},
Expand Down
5 changes: 4 additions & 1 deletion pkg/app_watcher/appset_watcher_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ import (
)

func initTestObjectsForAppSets(t *testing.T) *ApplicationSetWatcher {
ctx, cancel := context.WithCancel(context.Background())
defer cancel()

cfg, err := config.New()
cfg.AdditionalAppsNamespaces = []string{"*"}
// Handle the error appropriately, e.g., log it or fail the test
Expand Down Expand Up @@ -54,7 +57,7 @@ func initTestObjectsForAppSets(t *testing.T) *ApplicationSetWatcher {
vcsToArgoMap: appdir.NewVcsToArgoMap("vcs-username"),
}

appInformer, appLister := ctrl.newApplicationSetInformerAndLister(time.Second*1, cfg)
appInformer, appLister := ctrl.newApplicationSetInformerAndLister(time.Second*1, cfg, ctx)
ctrl.appInformer = appInformer
ctrl.appLister = appLister
return ctrl
Expand Down

0 comments on commit 77d2ff8

Please sign in to comment.