-
Notifications
You must be signed in to change notification settings - Fork 6.4k
fix: Operation has completed with phase: Running #7482
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
Conversation
Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>
Codecov Report
@@ Coverage Diff @@
## master #7482 +/- ##
==========================================
- Coverage 41.37% 41.36% -0.02%
==========================================
Files 161 161
Lines 21595 21602 +7
==========================================
Hits 8936 8936
- Misses 11397 11403 +6
- Partials 1262 1263 +1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with two minor comments.
controller/appcontroller.go
Outdated
if app.Operation != nil { | ||
// If we get here, we are about process an operation but we cannot rely on informer since it might has stale data. | ||
// So always retrieve the latest version to ensure it is not stale to avoid unnecessary syncing. | ||
// This code should be deleted when https://github.com/argoproj/argo-cd/pull/6294 is implemented. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#6294 is merged - I'm a little confused by this comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! I just reverted PR that introduced informer usage but forgot to remove obsolete comment. Fixed
controller/appcontroller.go
Outdated
} | ||
|
||
if app.Operation != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these if-clauses could be merged?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. done.
Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for review @jannfis . PTAL
controller/appcontroller.go
Outdated
if app.Operation != nil { | ||
// If we get here, we are about process an operation but we cannot rely on informer since it might has stale data. | ||
// So always retrieve the latest version to ensure it is not stale to avoid unnecessary syncing. | ||
// This code should be deleted when https://github.com/argoproj/argo-cd/pull/6294 is implemented. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! I just reverted PR that introduced informer usage but forgot to remove obsolete comment. Fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* fix: Operation has completed with phase: Running Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>
* fix(ui): Add Error Boundary around Extensions and comply with new Extensions API (argoproj#7215) * fix: Add error boundary around Extensions and change path where UI looks for extensions Signed-off-by: Remington Breeze <remington@breeze.software> * Add error message to error boundary Signed-off-by: Remington Breeze <remington@breeze.software> * docs: Kustomize load_restrictor -> load-restrictor (argoproj#7358) Signed-off-by: Jan-Otto Kröpke <joe@adorsys.de> * docs: update v2.3+ roadmap (argoproj#7353) * docs: update v2.3+ roadmap Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com> * Address reviewer notes: Add 'Merge Argo CD Image Updater into Argo CD' and 'Multi-tenancy improvements' Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com> * fix: remove not existing repo (argoproj#7280) * remove not existing repo Signed-off-by: pashavictorovich <pavel@codefresh.io> * fix test Signed-off-by: pashavictorovich <pavel@codefresh.io> * fix: Application status panel shows Syncing instead of Deleting (argoproj#7486) Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com> * fix: Operation has completed with phase: Running (argoproj#7482) * fix: Operation has completed with phase: Running Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com> * Bump version to 2.1.4 * Bump version to 2.1.4 * fix: Invalid memory address or nil pointer dereference in processRequestedAppOperation (argoproj#7501) Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com> * Bump version to 2.1.5 * Bump version to 2.1.5 * fix: supporting OCI dependencies. Fixes argoproj#6062 (argoproj#6994) * fix: supporting OCI dependencies Signed-off-by: Mohammad Yosefpor <myusefpur@gmail.com> * chore: add org to USERS.md Signed-off-by: Mohammad Yosefpor <myusefpur@gmail.com> * fix(tests): remove invalid TestRepoPermission e2e test Signed-off-by: Mohammad Yosefpor <myusefpur@gmail.com> * fix: don't use revision caching during app creation (argoproj#7508) Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com> * Bump version to 2.1.6 * Bump version to 2.1.6 * Fix: Kuberenetes manifest to have new Github.com ssh known host keys for ArgoCD deployments (argoproj#7722) * Kuberenetes manifest to have new ssh known host keys for ArgoCD deployments https://github.blog/2021-09-01-improving-git-protocol-security-github/ Signed-off-by: smark88 <msarcevicz@influxdata.com> * added to docs Signed-off-by: smark88 <msarcevicz@influxdata.com> * fix: regenerate manifests using 'make manifests' Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com> Co-authored-by: Alexander Matyushentsev <AMatyushentsev@gmail.com> * Bump version to 2.1.7 * Bump version to 2.1.7 * fix: upgraded gitops engine to v0.4.2 (fixes argoproj#7561) Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com> * fix: env vars to tune cluster cache were broken (argoproj#7779) Signed-off-by: Jesse Suen <jesse@akuity.io> * fix nil point (argoproj#7905) Signed-off-by: jomenxiao <jomenxiao@gmail.com> * fix: issue with keepalive (argoproj#7861) * fix issue with keepalive Signed-off-by: pashavictorovich <pavel@codefresh.io> * empty commit Signed-off-by: pashavictorovich <pavel@codefresh.io> * Bump version to 2.1.8 * Bump version to 2.1.8 * Merge pull request from GHSA-63qx-x74g-jcr7 Signed-off-by: jannfis <jann@mistrust.net> * Bump version to 2.1.9 * Bump version to 2.1.9 * fix: Resolve symlinked value files correctly (argoproj#8387) * fix: Resolve symlinked value files correctly Signed-off-by: jannfis <jann@mistrust.net> * fix: Resolve symlinked value files correctly Signed-off-by: jannfis <jann@mistrust.net> * Bump version to 2.1.10 * Bump version to 2.1.10 * add CODEFRESH=true to e2e workflow * codegen * fix e2e Co-authored-by: Remington Breeze <remington@breeze.software> Co-authored-by: Jan-Otto Kröpke <github@jkroepke.de> Co-authored-by: Alexander Matyushentsev <Alexander_Matyushentsev@intuit.com> Co-authored-by: pasha-codefresh <pavel@codefresh.io> Co-authored-by: argo-bot <argoproj@gmail.com> Co-authored-by: Mohammad Yosefpor <47300215+m-yosefpor@users.noreply.github.com> Co-authored-by: Mark Sarcevicz <47335998+smark88@users.noreply.github.com> Co-authored-by: Alexander Matyushentsev <AMatyushentsev@gmail.com> Co-authored-by: Jesse Suen <jessesuen@users.noreply.github.com> Co-authored-by: jomenxiao <jomenxiao@gmail.com> Co-authored-by: jannfis <jann@mistrust.net>
Signed-off-by: Alexander Matyushentsev AMatyushentsev@gmail.com
Fixes #5592
In v2.1 app controller updates cached application instances in the informer storage after patching the app. This approach allows avoiding using the stale app in the cache if the controller is the only one managing them. However, in Argo CD applications are managed by API server and controller so we cannot assume that informer contains the most recent app.
PR switches app controller back to fetching fresh app using API