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

Recreate missing Secrets for RepositoryCredentials. #666

Merged
merged 1 commit into from
Oct 25, 2023

Conversation

jparsai
Copy link
Contributor

@jparsai jparsai commented Oct 11, 2023

Description:

This PR is to add a logic to recreate Secrets in Argo CD namespace if they are required by RepositoryCredentials but missing in Cluster for some reason.

JIRA Ticket:

https://issues.redhat.com/browse/GITOPSRVCE-740

Type of changes

  1. Refactored existing recreateClusterSecrets_ManagedEnvironments function.
  2. Created a new function recreateClusterSecrets which calls recreateClusterSecrets_ManagedEnvironments function for creating missing secret for ManagedEnvironments and recreateClusterSecrets_RepositoryCredentials for RepositoryCredentials.

@openshift-ci openshift-ci bot requested review from jgwest and keithchong October 11, 2023 12:04
@codecov
Copy link

codecov bot commented Oct 11, 2023

Codecov Report

Attention: 32 lines in your changes are missing coverage. Please review.

Comparison is base (7f1c859) 62.64% compared to head (84b79c9) 62.72%.
Report is 12 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #666      +/-   ##
==========================================
+ Coverage   62.64%   62.72%   +0.07%     
==========================================
  Files          99       99              
  Lines       18162    18237      +75     
==========================================
+ Hits        11378    11439      +61     
- Misses       5518     5529      +11     
- Partials     1266     1269       +3     
Files Coverage Δ
...nt/controllers/argoproj.io/namespace_reconciler.go 68.38% <73.98%> (+1.84%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jgwest jgwest removed their request for review October 12, 2023 03:25

repositoryCredentials.Created_on = time.Now().Add(time.Duration(-(31 * time.Minute)))
err := dbq.UpdateRepositoryCredentials(ctx, &repositoryCredentials)
Expect(err).ToNot(HaveOccurred())
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this part here up to before the HaveLen check be commoned up with what is done in the previous test?

@@ -72,7 +72,8 @@ func (r *ApplicationReconciler) startTimerForNextCycle(ctx context.Context, name
cleanOrphanedCRsfromCluster_Operation(ctx, r.DB, r.Client, log)

// Recreate Secrets that are required by Applications, but missing from cluster.
recreateClusterSecrets(ctx, r.DB, r.Client, log)
recreateClusterSecrets_ManagedEnvironments(ctx, r.DB, r.Client, log)
recreateClusterSecrets_RepositoryCredentials(ctx, r.DB, r.Client, log)
Copy link
Contributor

@keithchong keithchong Oct 18, 2023

Choose a reason for hiding this comment

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

The new function has some code in common with the original recreateClusterSecrets_ManagedEnvironments function. I wonder if it can be commoned up? If not simple to refactor, then leave it as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is updated, PTAL.

@jparsai
Copy link
Contributor Author

jparsai commented Oct 18, 2023

An unrelated E2E is failing.
/test managed-gitops-e2e-tests

Copy link
Member

@jgwest jgwest left a comment

Choose a reason for hiding this comment

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

Looks perfect, thanks @jparsai, and thanks @keithchong for reviewing!

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 25, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jgwest, jparsai

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jgwest jgwest merged commit 04860d8 into redhat-appstudio:main Oct 25, 2023
13 checks passed
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 25, 2023

@jparsai: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/managed-gitops-e2e-tests 84b79c9 link unknown /test managed-gitops-e2e-tests

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants