Skip to content

fix: avoid spamming SyncPackage and InstallPackageRevision events#4934

Merged
negz merged 1 commit into
crossplane:masterfrom
phisco:dev/dont-spam-installpackagerevision-syncpackage-events
Oct 31, 2023
Merged

fix: avoid spamming SyncPackage and InstallPackageRevision events#4934
negz merged 1 commit into
crossplane:masterfrom
phisco:dev/dont-spam-installpackagerevision-syncpackage-events

Conversation

@phisco

@phisco phisco commented Oct 31, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description of your changes

Fixes #4933

I have:

Need help with this checklist? See the cheat sheet.

@phisco phisco requested review from a team and turkenh as code owners October 31, 2023 12:17
@phisco phisco requested review from bobh66 and negz October 31, 2023 12:17
bobh66
bobh66 approved these changes Oct 31, 2023
View reviewed changes

@bobh66 bobh66 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

negz
negz reviewed Oct 31, 2023
View reviewed changes
Comment thread internal/controller/pkg/manager/reconciler.go Outdated
@phisco phisco force-pushed the dev/dont-spam-installpackagerevision-syncpackage-events branch from 982b918 to 5202ef2 Compare October 31, 2023 18:27
jbw976
jbw976 reviewed Oct 31, 2023
View reviewed changes

@jbw976 jbw976 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

is this a regression from previous releases? 🤔

negz
negz reviewed Oct 31, 2023
View reviewed changes
Comment thread internal/controller/pkg/manager/reconciler.go Outdated
@negz

negz commented Oct 31, 2023

Copy link
Copy Markdown
Member

is this a regression from previous releases?

@jbw976 My guess would be no. I suspect using pull: Always is uncommon, which is why we haven't noticed this before.

@phisco

phisco commented Oct 31, 2023

Copy link
Copy Markdown
Contributor Author

is this a regression from previous releases? 🤔

Nope, 1.13.2 emits 1 InstallPackageRevision per minute too with packagePullPolicy set to Always. 1.12 will too. I'll set backport labels for both of them.

@phisco
fix: avoid spamming SyncPackage and InstallPackageRevision events
37e8ce4 
Signed-off-by: Philippe Scorsolini <p.scorsolini@gmail.com>
@phisco phisco force-pushed the dev/dont-spam-installpackagerevision-syncpackage-events branch from 5202ef2 to 37e8ce4 Compare October 31, 2023 18:49
@jbw976

jbw976 commented Oct 31, 2023

Copy link
Copy Markdown
Member

I suspect using pull: Always is uncommon, which is why we haven't noticed this before.

My personal preference is to not take changes (and churn) this late before the release, esp. if the code is a common/hot path but it isn't affecting a common user scenarios. But the maintainer team can have final say about including in v1.14 or not 🙇

@negz

negz commented Oct 31, 2023

Copy link
Copy Markdown
Member

@phisco and I discussed with @jbw976 offline and I think we're in agreement that this is low-risk enough to merge (famous last words?). We do definitely agree with the sentiment WRT last minute changes though.

@negz negz merged commit 1cd7c6d into crossplane:master Oct 31, 2023
@github-actions github-actions Bot mentioned this pull request Oct 31, 2023
Merged
@github-actions

github-actions Bot commented Oct 31, 2023

Copy link
Copy Markdown

Successfully created backport PR for release-1.12:

@github-actions

github-actions Bot commented Oct 31, 2023

Copy link
Copy Markdown

Successfully created backport PR for release-1.13:

This was referenced Oct 31, 2023
Merged
Merged
@github-actions

github-actions Bot commented Oct 31, 2023

Copy link
Copy Markdown

Successfully created backport PR for release-1.14:

@phisco phisco mentioned this pull request Nov 28, 2023
Merged
6 tasks
@phisco phisco deleted the dev/dont-spam-installpackagerevision-syncpackage-events branch May 6, 2026 12:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jbw976 jbw976 jbw976 left review comments

@negz negz negz approved these changes

@bobh66 bobh66 bobh66 approved these changes

@turkenh turkenh Awaiting requested review from turkenh

Assignees

No one assigned

Labels

backport release-1.12 backport release-1.13 backport release-1.14

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Functions get thousands of InstallPackageRevision events

4 participants

@phisco @negz @jbw976 @bobh66