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

ACM-5994 Clear cache and catch update error #253

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

o-farag
Copy link
Contributor

@o-farag o-farag commented Jul 10, 2023

Description of the change(s):

  • Clear cache and return err on failed deployment update

Why do we need this PR:

  • Failed deployment update error may fall through and not reinstall with correct arguments. Catch error and clear cache

Issue reference:

Test API/Unit - Success

?       github.com/stolostron/hypershift-addon-operator/cmd     [no test files]
ok      github.com/stolostron/hypershift-addon-operator/pkg/agent       12.513s coverage: 71.3% of statements
ok      github.com/stolostron/hypershift-addon-operator/pkg/install     175.386s        coverage: 84.9% of statements
ok      github.com/stolostron/hypershift-addon-operator/pkg/manager     120.042s        coverage: 60.6% of statements
ok      github.com/stolostron/hypershift-addon-operator/pkg/metrics     0.006s  coverage: 100.0% of statements
?       github.com/stolostron/hypershift-addon-operator/pkg/util        [no test files]

@openshift-ci
Copy link

openshift-ci bot commented Jul 10, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci
Copy link

openshift-ci bot commented Jul 10, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: o-farag
Once this PR has been reviewed and has the lgtm label, please assign zhujian7 for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@o-farag
Copy link
Contributor Author

o-farag commented Jul 10, 2023

/test all

@o-farag o-farag self-assigned this Jul 10, 2023
@o-farag o-farag marked this pull request as ready for review July 10, 2023 18:35
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@@ -175,3 +176,11 @@ func (c *UpgradeController) configmapDataChanged(oldCM, newCM corev1.ConfigMap,
}
return false
}

func (c* UpgradeController) clearCache() {
Copy link
Contributor

Choose a reason for hiding this comment

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

When we clear cache here, should we also remove locally saved secrets and configmaps as well?

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 don't think we need to remove them. Having a mismatch between the cache and locally saved secrets should cause a reconcile, and https://github.com/o-farag/hypershift-addon-operator/blob/c891a92db4f12cb78a4d454f3bf77a058ca23854/pkg/install/hypershift.go#L552 will update the secrets if anything changes

@openshift-merge-robot
Copy link

PR needs rebase.

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-sigs/prow repository.

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