Skip to content
Open
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: 7 additions & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,12 @@ endif
SHELL = /usr/bin/env bash -o pipefail
.SHELLFLAGS = -ec

# GINKGO_VERSION is the version of ginkgo to use.
# Pick ginkgo version from go.mod file.
# Update this command when ginkgo version is updated in go.mod file.
# example: go list -m -f '{{.Version}}' github.com/onsi/ginkgo/v3
GINKGO_VERSION := $(shell go list -m -f '{{.Version}}' github.com/onsi/ginkgo/v2)

.PHONY: all
all: build

Expand Down Expand Up @@ -247,7 +253,7 @@ kustomize: ## Download kustomize locally if necessary.
GINKGO_CLI = $(shell pwd)/bin/ginkgo
.PHONY: ginkgo
ginkgo: ## Download ginkgo locally if necessary.
$(call go-get-tool,$(GINKGO_CLI),github.com/onsi/ginkgo/v2/ginkgo@v2.29.0)
$(call go-get-tool,$(GINKGO_CLI),github.com/onsi/ginkgo/v2/ginkgo@$(GINKGO_VERSION))


# go-get-tool will 'go install' any package $2 and install it to $1.
Expand Down
6 changes: 3 additions & 3 deletions test/e2e/argocd_route_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,15 +33,15 @@ var _ = Describe("Argo CD ConsoleLink controller", func() {
route := &routev1.Route{}
consoleLink := &console.ConsoleLink{}

It("Argocd route is present", func() {
It("Argocd route is present", Label("openshift"), func() {
checkIfPresent(types.NamespacedName{Name: argoCDRouteName, Namespace: argoCDNamespace}, route)
})

It("ConsoleLink is created", func() {
It("ConsoleLink is created", Label("openshift"), func() {
checkIfPresent(types.NamespacedName{Name: consoleLinkName}, consoleLink)
})

It("ConsoleLink and argocd route should match", func() {
It("ConsoleLink and argocd route should match", Label("openshift"), func() {
Eventually(func() error {
err := k8sClient.Get(context.TODO(), types.NamespacedName{Name: consoleLinkName}, consoleLink)
if err != nil {
Expand Down
193 changes: 107 additions & 86 deletions test/openshift/e2e/ginkgo/fixture/fixture.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,14 +60,16 @@ func EnsureParallelCleanSlate() {

// Finally, wait for default openshift-gitops instance to be ready
// - Parallel tests should not write to any resources in 'openshift-gitops' namespace (sequential only), but they are allowed to read from them.
defaultOpenShiftGitOpsArgoCD := &argov1beta1api.ArgoCD{
ObjectMeta: metav1.ObjectMeta{Name: "openshift-gitops", Namespace: "openshift-gitops"},
}
err := k8sClient.Get(context.Background(), client.ObjectKeyFromObject(defaultOpenShiftGitOpsArgoCD), defaultOpenShiftGitOpsArgoCD)
Expect(err).ToNot(HaveOccurred())

Eventually(defaultOpenShiftGitOpsArgoCD, "5m", "5s").Should(argocd.BeAvailableWithCustomSleepTime(3 * time.Second))
// default instance only runs on openshift clusters
if RunningOnOpenShift() {
defaultOpenShiftGitOpsArgoCD := &argov1beta1api.ArgoCD{
ObjectMeta: metav1.ObjectMeta{Name: "openshift-gitops", Namespace: "openshift-gitops"},
}
err := k8sClient.Get(context.Background(), client.ObjectKeyFromObject(defaultOpenShiftGitOpsArgoCD), defaultOpenShiftGitOpsArgoCD)
Expect(err).ToNot(HaveOccurred())

Eventually(defaultOpenShiftGitOpsArgoCD, "5m", "5s").Should(argocd.BeAvailableWithCustomSleepTime(3 * time.Second))
}
// Unlike sequential clean slate, parallel clean slate cannot assume that there are no other tests running. This limits our ability to clean up old test artifacts.
}

Expand Down Expand Up @@ -107,101 +109,104 @@ func EnsureSequentialCleanSlateWithError() error {
return err
}

// don't wait for openshift-gitops ArgoCD to exist, if it is on xKS cluster
// wait for openshift-gitops ArgoCD to exist, if it doesn't already
defaultOpenShiftGitOpsArgoCD := &argov1beta1api.ArgoCD{
ObjectMeta: metav1.ObjectMeta{Name: "openshift-gitops", Namespace: "openshift-gitops"},
}
Eventually(defaultOpenShiftGitOpsArgoCD, "3m", "5s").Should(k8s.ExistByName())

// Ensure that default state of ArgoCD CR in openshift-gitops is restored
if err := updateWithoutConflict(defaultOpenShiftGitOpsArgoCD, func(obj client.Object) {
argocdObj, ok := obj.(*argov1beta1api.ArgoCD)
Expect(ok).To(BeTrue())

// HA should be disabled by default
argocdObj.Spec.HA.Enabled = false

// .spec.monitoring.disableMetrics should be nil by default
argocdObj.Spec.Monitoring.DisableMetrics = nil

// Ensure that api server route has not been disabled, nor exposed via different settings
argocdObj.Spec.Server.Route = argov1beta1api.ArgoCDRouteSpec{
Enabled: true,
TLS: nil,
// TLS: &routev1.TLSConfig{
// Termination: routev1.TLSTerminationReencrypt,
// InsecureEdgeTerminationPolicy: routev1.InsecureEdgeTerminationPolicyRedirect,
// },
if RunningOnOpenShift() {
defaultOpenShiftGitOpsArgoCD := &argov1beta1api.ArgoCD{
ObjectMeta: metav1.ObjectMeta{Name: "openshift-gitops", Namespace: "openshift-gitops"},
}
Eventually(defaultOpenShiftGitOpsArgoCD, "3m", "5s").Should(k8s.ExistByName())

// Ensure that default state of ArgoCD CR in openshift-gitops is restored
if err := updateWithoutConflict(defaultOpenShiftGitOpsArgoCD, func(obj client.Object) {
argocdObj, ok := obj.(*argov1beta1api.ArgoCD)
Expect(ok).To(BeTrue())

// HA should be disabled by default
argocdObj.Spec.HA.Enabled = false

// .spec.monitoring.disableMetrics should be nil by default
argocdObj.Spec.Monitoring.DisableMetrics = nil

// Ensure that api server route has not been disabled, nor exposed via different settings
argocdObj.Spec.Server.Route = argov1beta1api.ArgoCDRouteSpec{
Enabled: true,
TLS: nil,
// TLS: &routev1.TLSConfig{
// Termination: routev1.TLSTerminationReencrypt,
// InsecureEdgeTerminationPolicy: routev1.InsecureEdgeTerminationPolicyRedirect,
// },
}

// Reset app controller processors to default
argocdObj.Spec.Controller.Processors = argov1beta1api.ArgoCDApplicationControllerProcessorsSpec{}
// Reset app controller processors to default
argocdObj.Spec.Controller.Processors = argov1beta1api.ArgoCDApplicationControllerProcessorsSpec{}

// Reset repo server replicas to default
argocdObj.Spec.Repo.Replicas = nil
// Reset repo server replicas to default
argocdObj.Spec.Repo.Replicas = nil

// Reset source namespaces
argocdObj.Spec.SourceNamespaces = nil
argocdObj.Spec.ApplicationSet.SourceNamespaces = nil
// Reset source namespaces
argocdObj.Spec.SourceNamespaces = nil
argocdObj.Spec.ApplicationSet.SourceNamespaces = nil

}); err != nil {
return err
}
}); err != nil {
return err
}

gitopsService := &gitopsoperatorv1alpha1.GitopsService{
ObjectMeta: metav1.ObjectMeta{Name: "cluster"},
}
if err := k8sClient.Get(ctx, client.ObjectKeyFromObject(gitopsService), gitopsService); err != nil {
return err
}
gitopsService := &gitopsoperatorv1alpha1.GitopsService{
ObjectMeta: metav1.ObjectMeta{Name: "cluster"},
}
if err := k8sClient.Get(ctx, client.ObjectKeyFromObject(gitopsService), gitopsService); err != nil {
return err
}

// Ensure that run on infra is disabled: some tests will enable it
if err := updateWithoutConflict(gitopsService, func(obj client.Object) {
goObj, ok := obj.(*gitopsoperatorv1alpha1.GitopsService)
Expect(ok).To(BeTrue())
// Ensure that run on infra is disabled: some tests will enable it
if err := updateWithoutConflict(gitopsService, func(obj client.Object) {
goObj, ok := obj.(*gitopsoperatorv1alpha1.GitopsService)
Expect(ok).To(BeTrue())

goObj.Spec.NodeSelector = nil
goObj.Spec.RunOnInfra = false
goObj.Spec.Tolerations = nil
}); err != nil {
return err
}
goObj.Spec.NodeSelector = nil
goObj.Spec.RunOnInfra = false
goObj.Spec.Tolerations = nil
}); err != nil {
return err
}

// Clean up old cluster-scoped role from 1-034
_ = k8sClient.Delete(ctx, &rbacv1.ClusterRole{ObjectMeta: metav1.ObjectMeta{Name: "custom-argocd-role"}})
// Clean up old cluster-scoped role from 1-034
_ = k8sClient.Delete(ctx, &rbacv1.ClusterRole{ObjectMeta: metav1.ObjectMeta{Name: "custom-argocd-role"}})
Comment on lines +174 to +175

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Keep cluster-agnostic cleanup outside the OpenShift gate.

custom-argocd-role is a standard Kubernetes ClusterRole. On xKS, this change leaves it behind between sequential tests, allowing stale state to contaminate later runs. Move its deletion before Line 114; keep only OpenShift-specific resources inside the gate.

As per path instructions, “Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.”

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/openshift/e2e/ginkgo/fixture/fixture.go` around lines 174 - 175, Move
the custom-argocd-role deletion in the fixture cleanup flow before the
OpenShift-specific gate, near the setup preceding line 114. Keep deletion of
this standard ClusterRole unconditional so xKS runs also remove stale state,
while leaving only OpenShift-specific resource cleanup inside the gate.

Source: Path instructions


// Delete all existing RolloutManagers in openshift-gitops Namespace
var rolloutManagerList rolloutmanagerv1alpha1.RolloutManagerList
if err := k8sClient.List(ctx, &rolloutManagerList, client.InNamespace("openshift-gitops")); err != nil {
return err
}
for _, rm := range rolloutManagerList.Items {
if err := k8sClient.Delete(ctx, &rm); err != nil {
// Delete all existing RolloutManagers in openshift-gitops Namespace
var rolloutManagerList rolloutmanagerv1alpha1.RolloutManagerList
if err := k8sClient.List(ctx, &rolloutManagerList, client.InNamespace("openshift-gitops")); err != nil {
return err
}
}
for _, rm := range rolloutManagerList.Items {
if err := k8sClient.Delete(ctx, &rm); err != nil {
return err
}
}

// Delete 'restricted-dropcaps' which is created by at least one test
scc := &securityv1.SecurityContextConstraints{
ObjectMeta: metav1.ObjectMeta{
Name: "restricted-dropcaps",
},
}
if err := k8sClient.Delete(ctx, scc); err != nil {
if !apierr.IsNotFound(err) {
return err
// Delete 'restricted-dropcaps' which is created by at least one test
scc := &securityv1.SecurityContextConstraints{
ObjectMeta: metav1.ObjectMeta{
Name: "restricted-dropcaps",
},
}
if err := k8sClient.Delete(ctx, scc); err != nil {
if !apierr.IsNotFound(err) {
return err
}
// Otherwise, expected error if it doesn't exist.
}
// Otherwise, expected error if it doesn't exist.
}

// Finally, wait for default openshift-gitops instance to be ready.
failure := InterceptGomegaFailure(func() {
Eventually(defaultOpenShiftGitOpsArgoCD, "5m", "5s").Should(argocd.BeAvailable())
})
// Output debug information on argo startup failure
if failure != nil {
OutputDebug(defaultOpenShiftGitOpsArgoCD.Namespace)
Fail(failure.Error())
// Finally, wait for default openshift-gitops instance to be ready.
failure := InterceptGomegaFailure(func() {
Eventually(defaultOpenShiftGitOpsArgoCD, "5m", "5s").Should(argocd.BeAvailable())
})
// Output debug information on argo startup failure
if failure != nil {
OutputDebug(defaultOpenShiftGitOpsArgoCD.Namespace)
Fail(failure.Error())
}
}
return nil
}
Expand Down Expand Up @@ -988,6 +993,22 @@ func RunningOnOpenShift() bool {
return openshiftAPIsFound > 5 // I picked 5 as an arbitrary number, could also just be 1
}

// IsOperatorRunningOnOLM returns true if the operator is running on OLM, false otherwise.
func IsOperatorRunningOnOLM() bool {
k8sClient, _ := utils.GetE2ETestKubeClient()

crdList := crdv1.CustomResourceDefinitionList{}
Expect(k8sClient.List(context.Background(), &crdList)).To(Succeed())

olmAPIsFound := 0
for _, crd := range crdList.Items {
if strings.Contains(crd.Spec.Group, "operators.coreos.com") {
olmAPIsFound++
}
}
return olmAPIsFound > 0
Comment on lines +996 to +1009

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
rg -n -C4 --type go '\bIsOperatorRunningOnOLM\s*\(' .
rg -n -C4 --type go 'ClusterServiceVersion|Subscription|OwnerReferences|gitops-operator' test/openshift/e2e/ginkgo

Repository: redhat-developer/gitops-operator

Length of output: 50389


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '\n## IsOperatorRunningOnOLM definition\n'
sed -n '980,1035p' test/openshift/e2e/ginkgo/fixture/fixture.go

printf '\n## OLM-related helpers/usages in fixture tree\n'
rg -n -C3 'IsOperatorRunningOnOLM|ClusterServiceVersion|Subscription|OwnerReferences|ManagedFields|operator Subscription|operator Deployment' test/openshift/e2e/ginkgo/fixture test/openshift/e2e/ginkgo/sequential test/openshift/e2e/ginkgo/parallel

Repository: redhat-developer/gitops-operator

Length of output: 50388


test/openshift/e2e/ginkgo/fixture/fixture.go:996-1009 Check the operator’s install source, not cluster-wide OLM CRDs. This returns true on any OLM-enabled cluster, so a manually deployed operator can be misclassified as OLM-managed. Use the operator’s Subscription/CSV or ownership chain instead.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/openshift/e2e/ginkgo/fixture/fixture.go` around lines 996 - 1009, Update
IsOperatorRunningOnOLM to determine whether the tested operator is OLM-managed
by inspecting its relevant Subscription/CSV or ownership chain, rather than
counting cluster-wide CRDs whose group contains operators.coreos.com. Preserve
the false result for manually deployed operators and return true only when the
operator’s own resources demonstrate OLM ownership.

Source: Path instructions

}

//nolint:unused
func outputPodLog(podSubstring string) {
k8sClient, _, err := utils.GetE2ETestKubeClientWithError()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ var _ = Describe("GitOps Operator Parallel E2E Tests", func() {
ctx = context.Background()
})

It("Ensure dex spec field can be converted between ArgoCD v1alpha1 and v1beta1", func() {
It("Ensure dex spec field can be converted between ArgoCD v1alpha1 and v1beta1", Label("openshift"), Label("conversionWebhook"), func() {

if fixture.EnvLocalRun() {
Skip("Conversion via webhook requires the operator to be running on the cluster, which is not the case for a local run")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,10 @@ var _ = Describe("GitOps Operator Parallel E2E Tests", func() {
ctx = context.Background()
})

It("verifies expected behaviour of ArgoCD CR when dex and keycloak are both specified in v1alpha1 API", func() {
It("verifies expected behaviour of ArgoCD CR when dex and keycloak are both specified in v1alpha1 API", Label("conversionWebhook", "openshift"), func() {

if fixture.EnvLocalRun() {
Skip("Conversion via webhook requires the operator to be running on the cluster, which is not the case for a local run")
Skip("Conversion via webhook requires the operator to be running on the openshift cluster, which is not the case for a local or on xKS cluster")
return
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,16 +17,14 @@ var _ = Describe("GitOps Operator Parallel E2E Tests", func() {
fixture.EnsureParallelCleanSlate()
})

It("verifies ConsoleLink exists and has expected content", func() {
It("verifies ConsoleLink exists and has expected content", Label("openshift"), func() {

consoleLink := &consolev1.ConsoleLink{ObjectMeta: metav1.ObjectMeta{
Name: "argocd",
}}
Eventually(consoleLink).Should(k8sFixture.ExistByName())
Expect(string(consoleLink.Spec.Location)).To(Equal("ApplicationMenu"))
Expect(consoleLink.Spec.Text).To(Equal("Cluster Argo CD"))

})

})
})
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ var _ = Describe("GitOps Operator Parallel E2E Tests", func() {
ctx = context.Background()
})

It("verifies v1beta1 ArgoCD CR containing Dex SSO values can be converted to v1alpha1", func() {
It("verifies v1beta1 ArgoCD CR containing Dex SSO values can be converted to v1alpha1", Label("conversionWebhook", "openshift"), func() {

if fixture.EnvLocalRun() {
Skip("Conversion via webhook requires the operator to be running on the cluster, which is not the case for a local run")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ var _ = Describe("GitOps Operator Parallel E2E Tests", func() {
fixture.EnsureParallelCleanSlate()
})

It("verifies that default ServiceMonitors exist in openshift-gitops and PrometheusRule ArgoCDSyncAlert exists", func() {
It("verifies that default ServiceMonitors exist in openshift-gitops and PrometheusRule ArgoCDSyncAlert exists", Label("openshift"), func() {

By("verifying openshift-gitops ServiceMonitor exists and has expected values")
openshiftGitOpsSM := &monitoringv1.ServiceMonitor{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ var _ = Describe("GitOps Operator Parallel E2E Tests", func() {
ctx = context.Background()
})

It("ensures that certificates can be confirmed on server and webhook Routes", func() {
It("ensures that certificates can be confirmed on server and webhook Routes", Label("openshift"), func() {

fixture.EnsureRunningOnOpenShift()

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ var _ = Describe("GitOps Operator Parallel E2E Tests", func() {
fixture.OutputDebugOnFail(nsTest_1_23_custom)
})

It("verifying ArgoCD .spec.repo AutoTLS and verifyTLS work as expected", func() {
It("verifying ArgoCD .spec.repo AutoTLS and verifyTLS work as expected", Label("openshift"), func() {

By("creating a namespace scoped Argo instance with AutoTLS set to 'openshift'")

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ var _ = Describe("GitOps Operator Parallel E2E Tests", func() {
ctx = context.Background()
})

It("verifies Argo CD Server's Route can be enabled with TLSTerminationReencrypt", func() {
It("verifies Argo CD Server's Route can be enabled with TLSTerminationReencrypt", Label("openshift"), func() {

By("creating namespace-scoped Argo CD instance with rencrypt Route")

Expand Down
Loading
Loading