-
Notifications
You must be signed in to change notification settings - Fork 96
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
Add metadata key for holding an arbitrary identification token #544
Conversation
Have discussed this more with @darkowlzz. How we think this should work is that it would introduce a new This would cover for the alternative approach I documented in fluxcd/notification-controller#518 (comment), while allowing the source-controller to make use of this as well which hasn't a "checksum" concept but rather a reflection of observed values in full. |
@hiddeco nice! That's indeed much better. Would the revision be included in this token, or kept as a separate thing as it is today? |
That's up to the emitting controller to decide, the notification-controller should just consume it as part of the key that handles rate limiting. |
Yep my question is specifically about what to do on the notification-controller side. Right now it includes the |
We would use both, as they represent different things (source being used versus declarative configuration). |
@hiddeco I updated the PR 👌 |
@stefanprodan @darkowlzz please take a look. |
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!
Please rebase and also maybe update the PR description to not accidentally close fluxcd/helm-controller#678 as I think that requires changes in helm-controller like in fluxcd/helm-controller#681 .
Signed-off-by: Matheus Pimenta <[email protected]>
@darkowlzz I updated the PR description, please let me know if it works |
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
Part of the PRs for fluxcd/helm-controller#678 alongside fluxcd/helm-controller#681 and fluxcd/notification-controller#518