Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix(appset): informer is not a kubernetes informer (#18905) #19618

Merged
merged 1 commit into from
Aug 22, 2024

Conversation

crenshaw-dev
Copy link
Member

updateCache was introduced in #15514 to avoid a bug where the appset controller would delete an app's status field.

The PR had to changes: 1) use patch instead of update and 2) write updates back to the informer to avoid stale data.

I think the first change is probably enough to fix the bug.

With the latest controller runtime code, I'm not seeing a way to access the underlying data store to do write-backs. But maybe at this point it's not necessary anyway.

Fixes #18905

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
Copy link

bunnyshell bot commented Aug 21, 2024

❌ Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

  • 🚀 /bns:deploy to deploy the environment

Copy link

bunnyshell bot commented Aug 21, 2024

❌ Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

  • 🚀 /bns:deploy to deploy the environment

Copy link

codecov bot commented Aug 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 55.90%. Comparing base (effbdc9) to head (fb5972e).
Report is 12 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #19618      +/-   ##
==========================================
+ Coverage   55.85%   55.90%   +0.04%     
==========================================
  Files         316      316              
  Lines       43790    43776      -14     
==========================================
+ Hits        24459    24473      +14     
+ Misses      16779    16756      -23     
+ Partials     2552     2547       -5     

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

@crenshaw-dev
Copy link
Member Author

@alexmt does this seem reasonable, or should we dig further to find out how to write back to the informer?

@alexmt
Copy link
Collaborator

alexmt commented Aug 22, 2024

I've wasted a few hours and also could not find a way to access the underlying informer. PR makes sense to get rid of error message.

@crenshaw-dev crenshaw-dev marked this pull request as ready for review August 22, 2024 00:21
@crenshaw-dev crenshaw-dev requested a review from a team as a code owner August 22, 2024 00:21
@crenshaw-dev
Copy link
Member Author

Access the Informer Store with this One Easy Trick Kubernetes Maintainers Don't Want You to Know!

Copy link
Member

@ishitasequeira ishitasequeira left a comment

Choose a reason for hiding this comment

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

The change LGTM!!

@crenshaw-dev crenshaw-dev merged commit 3cdce83 into argoproj:master Aug 22, 2024
28 of 29 checks passed
@crenshaw-dev crenshaw-dev deleted the fix-failing-cast branch August 22, 2024 04:15
@crenshaw-dev
Copy link
Member Author

/cherry-pick release-2.12

gcp-cherry-pick-bot bot pushed a commit that referenced this pull request Aug 22, 2024
Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
crenshaw-dev added a commit that referenced this pull request Aug 22, 2024
…19636)

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
Co-authored-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
pasha-codefresh pushed a commit to pasha-codefresh/argo-cd that referenced this pull request Oct 9, 2024
…rgoproj#19618)

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

appset: Error informer is not a kubernetes informer when controller generate applications
3 participants