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

Gitops 2857 fix kuttl test parallel 055 #940

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

ciiay
Copy link
Collaborator

@ciiay ciiay commented Jun 20, 2023

Signed off: @ciiay

What type of PR is this?
/kind bug

What does this PR do / why we need it:
For flaky kuttl test 1-055_validate_notification_controller
When notifications controller deployment was getting deleted, it got this error

message: >-
        pods "example-argocd-notifications-controller-78b644d677-b77pp" is
        forbidden: error looking up service account
        kuttl-test-whole-toad/example-argocd-argocd-notifications-controller:
        serviceaccount "example-argocd-argocd-notifications-controller" not
        found

Major updates for best practice:

Moved cr.Spec.Notifications.Enabled condition check from each resource reconciliation function into higher-level function, reconcileResources
Split the original resource reconcile function into two functions. One for reconciliation, another one for deletion.
Removed deletion from deleteNotificationsPred function, so that it's purely for predication of Notifications.Enabled value change.

Which issue(s) this PR fixes:

Fixes #? GITOPS-2857

How to test changes / Special notes to the reviewer:
kuttl test parallel/1-055_validate_notification_controller shouldn't be flaky or fail anymore. This PR works along with redhat-developer/gitops-operator#527 which adds teststep to wait for notification controller pod to be ready.

Note:
Corresponding test case doesn't need to change. This only adds a not found check before executing deletion of service account. I tried to update the test case, but since it's a race condition issue, deployment and rolebinding instance will always be deleted first before reconcile function deletes the service account.

@ciiay ciiay force-pushed the gitops-2857-fix-kuttl-test-parallel-055 branch 2 times, most recently from abb5d68 to 9228363 Compare June 21, 2023 17:18
@ciiay ciiay force-pushed the gitops-2857-fix-kuttl-test-parallel-055 branch 2 times, most recently from a4524cc to 62f2a54 Compare June 29, 2023 22:37
ciiay and others added 20 commits July 10, 2023 16:53
Signed-off-by: Yi Cai <[email protected]>
Signed-off-by: Yi Cai <[email protected]>
Signed-off-by: Yi Cai <[email protected]>
Signed-off-by: Yi Cai <[email protected]>
Signed-off-by: Yi Cai <[email protected]>
Signed-off-by: Yi Cai <[email protected]>
…ds (argoproj-labs#896)

* Remove deprecated dex & sso fields

Signed-off-by: Siddhesh Ghadi <[email protected]>

* Unify .status.sso

Signed-off-by: Siddhesh Ghadi <[email protected]>

* Add unit test for .status.sso unification

Signed-off-by: Siddhesh Ghadi <[email protected]>

* Add & fix e2e tests for unified status.sso

Signed-off-by: Siddhesh Ghadi <[email protected]>

* make bundle

Signed-off-by: Siddhesh Ghadi <[email protected]>

* Fix make bundle codegen ci failure

Signed-off-by: Siddhesh Ghadi <[email protected]>

* Fix keycloak status reconciliation

Signed-off-by: Siddhesh Ghadi <[email protected]>

* Case insensitive sso provider check

Signed-off-by: Siddhesh Ghadi <[email protected]>

* Do not block reconciliation due to sso failures

SSO is a non-critical component and we shouldn't block reconciliation on its failure

Signed-off-by: Siddhesh Ghadi <[email protected]>

* Remove extra .ToLower() calls on already defined sso value

Signed-off-by: Siddhesh Ghadi <[email protected]>

* Remove envVar & setEnvVarFunc feilds from dex unit tests

There are a lot of unit tests that have envVar & setEnvVarFunc field that was required for DISABLE_DEX testing.
These fields are not required in these tests anymore.

Signed-off-by: Siddhesh Ghadi <[email protected]>

* Fix ci failure

Signed-off-by: Siddhesh Ghadi <[email protected]>

---------

Signed-off-by: Siddhesh Ghadi <[email protected]>
Signed-off-by: Yi Cai <[email protected]>
* Update docs to reflect sso unification changes

Signed-off-by: Siddhesh Ghadi <[email protected]>

* Add a note about removed sso fields

Signed-off-by: Siddhesh Ghadi <[email protected]>

---------

Signed-off-by: Siddhesh Ghadi <[email protected]>
Signed-off-by: Yi Cai <[email protected]>
Fix typo in trigger on-sync-status-unknown for argocd-notifications-cm configMap.

Signed-off-by: Sirbu, Cristina <[email protected]>
Signed-off-by: Sirbu, Cristina <[email protected]>

---------

Signed-off-by: Arthur <[email protected]>
Signed-off-by: Sirbu, Cristina <[email protected]>
Signed-off-by: Sirbu, Cristina <[email protected]>
Co-authored-by: Arthur Vardevanyan <[email protected]>
Signed-off-by: Yi Cai <[email protected]>
Signed-off-by: Yi Cai <[email protected]>
@ciiay ciiay force-pushed the gitops-2857-fix-kuttl-test-parallel-055 branch from 9e26139 to 7196cb4 Compare July 10, 2023 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants