Skip to content

Refactor reconcilers and introduce v1beta2 API #435

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 26 commits into from
Dec 13, 2022
Merged

Refactor reconcilers and introduce v1beta2 API #435

merged 26 commits into from
Dec 13, 2022

Conversation

@stefanprodan
Copy link
Member

@stefanprodan stefanprodan commented Oct 27, 2022 •
edited
Loading

This PR refactors notification-controller to implement the new unified standards of Flux controllers (closes: #376) and documents the API usage in the new format (closes: #378, fixes: #433).

The API version v1beta2 is backwards compatible with v1beta1. The only changes are to the .status sub-resource and .status.conditions, which the controller will upgrade on its own. No user intervention is required besides bumping the version in the custom resource definitions.

API changes:

  • Deprecate Receiver.status.url in favor of .status.webhookPath
  • Replace the Stalled=True condition with Reconciling=True and Reason=ProgressingWithRetry on retryable failures
  • Add lastHandledReconcileAt to .status
  • Add spec.interval to periodically reconcile Providers and Receivers with their Secret references to surface config errors after initialisation.
  • The different status phases are now unit tested to ensure that the controller reflects the observed state correctly
  • Enforce a max length of 2048 for URL type fields such as address and proxy
  • Enforce a max length of 255 for the summary field (required by Slack and others)

Controller changes:

Bug fixes included in this PR:

  • Suspended resources where never finalised
  • Receivers were reconciled on .status updates instead of just .spec & ReconcileAt
  • On refs not found errors, resources weren't explicitly requeue
  • ReconcileAt was not handled at all, even if some types were reacting to it
  • The Stalled condition was added even if the controller could recover on the next try
  • Fix: Receiver webhooks revert apiVersion of ImageRepository to v1alpha1 #328

@pjbgf pjbgf added this to the GA milestone Oct 27, 2022
@stefanprodan stefanprodan force-pushed the api-v1beta2 branch 4 times, most recently from e5a9b99 to cca289d Compare October 29, 2022 10:07
@stefanprodan stefanprodan requested a review from a team October 31, 2022 11:09
@stefanprodan stefanprodan added enhancement New feature or request area/docs Documentation related issues and pull requests area/receiver Webhook receiver related issues and PRs area/alerting Alerting related issues and PRs labels Oct 31, 2022
@stefanprodan stefanprodan marked this pull request as ready for review October 31, 2022 11:09
@stefanprodan stefanprodan mentioned this pull request Nov 2, 2022
Closed
20 tasks
@hiddeco hiddeco self-requested a review November 2, 2022 13:12
somtochiama
somtochiama approved these changes Nov 2, 2022
View reviewed changes
Copy link
Member

@somtochiama somtochiama left a comment

Choose a reason for hiding this comment

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

I have tested that the commit status still works with this update

@stefanprodan stefanprodan force-pushed the api-v1beta2 branch 3 times, most recently from 68180be to 26f84f1 Compare November 3, 2022 09:19
pjbgf
pjbgf reviewed Nov 4, 2022
View reviewed changes
@pjbgf pjbgf mentioned this pull request Nov 4, 2022
Closed
22 tasks
darkowlzz
darkowlzz reviewed Nov 4, 2022
View reviewed changes
@stefanprodan stefanprodan mentioned this pull request Nov 7, 2022
Merged
darkowlzz
darkowlzz reviewed Nov 7, 2022
View reviewed changes
darkowlzz
darkowlzz reviewed Nov 7, 2022
View reviewed changes
This was referenced Nov 8, 2022
Merged
Merged
Merged
stefanprodan and others added 15 commits December 9, 2022 12:05
@stefanprodan @hiddeco
Issue warning events on reconciliation errors
ee2600a 
Signed-off-by: Stefan Prodan <[email protected]>
@stefanprodan @hiddeco
Refactor notifies to use Flux Event API v1beta1
caec764 
Signed-off-by: Stefan Prodan <[email protected]>
@stefanprodan @hiddeco
Log and emit events on successful reconciliation
012faa2 
Signed-off-by: Stefan Prodan <[email protected]>
@stefanprodan @hiddeco
Make interval optional
70c678f 
Signed-off-by: Stefan Prodan <[email protected]>
@stefanprodan @hiddeco
Validate the provider inline address and proxy
1a4a4a8 
Signed-off-by: Stefan Prodan <[email protected]>
@stefanprodan @hiddeco
Compact success events
c9a774f 
Signed-off-by: Stefan Prodan <[email protected]>
@stefanprodan @hiddeco
Deprecate Receiver.status.url in favor of .status.webhookPath
d16588a 
Signed-off-by: Stefan Prodan <[email protected]>
@stefanprodan @hiddeco
Use ProgressingWithRetry from fluxcd/pkg/apis/meta
5a0d6dd 
Signed-off-by: Stefan Prodan <[email protected]>
@stefanprodan @hiddeco
Stall providers with invalid spec
4aad301 
Signed-off-by: Stefan Prodan <[email protected]>
@stefanprodan @hiddeco
docs: Add status and events to examples
831785a 
Signed-off-by: Stefan Prodan <[email protected]>
@stefanprodan @hiddeco
docs: Add alert status specification
04f5288 
Signed-off-by: Stefan Prodan <[email protected]>
@stefanprodan @hiddeco
docs: Add commit status updates to providers spec
78fe519 
Signed-off-by: Stefan Prodan <[email protected]>
@stefanprodan @hiddeco
docs: Add Provider and Receiver status spec
20fa1a0 
Signed-off-by: Stefan Prodan <[email protected]>
@hiddeco
docs: improve Receiver spec
ef94247 
- Extensively document the various Receiver types, their validation
  mechanics and their caveats.
- Move various things around a bit to ensure we follow the same flow
  as the rewritten source-controller specs.
- Explicitly document the expected format of the Secret.
- Various other small rewordings and fixes.

Signed-off-by: Hidde Beydals <[email protected]>
@hiddeco
config: ensure event create/patch is registered
b0c02d7 
Signed-off-by: Hidde Beydals <[email protected]>
relu
relu approved these changes Dec 9, 2022
View reviewed changes
Copy link
Member

@relu relu left a comment

Choose a reason for hiding this comment

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

So much stuff here! Refactoring looks really nice ✔️

@hiddeco hiddeco force-pushed the api-v1beta2 branch 2 times, most recently from 502a898 to b07d09d Compare December 9, 2022 13:55
pjbgf
pjbgf approved these changes Dec 9, 2022
View reviewed changes
Copy link
Member

@pjbgf pjbgf left a comment

Choose a reason for hiding this comment

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

LGTM!

Great job @stefanprodan @darkowlzz @hiddeco! 👏 👏 👏

darkowlzz
darkowlzz approved these changes Dec 12, 2022
View reviewed changes
Copy link
Contributor

@darkowlzz darkowlzz left a comment

Choose a reason for hiding this comment

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

Left comments about two minor fixes in the docs.
Overall LGTM!

@hiddeco
docs: Rewrite portion of Provider spec
16012ce 
Plus addressing of a couple of nits.

The following must still be picked up at a later moment:

- More sections need to be moved from "working with" to "writing a ..."
- Documentation flow can likely be improved once the sections have been
  moved.
- Technical description of the behavior of the Provider types could be
  improved, this should be easier to do when everything has the same
  format.

Signed-off-by: Hidde Beydals <[email protected]>
@hiddeco hiddeco merged commit 5dc6124 into main Dec 13, 2022
@hiddeco hiddeco deleted the api-v1beta2 branch December 13, 2022 13:27
@darkowlzz darkowlzz mentioned this pull request Jun 1, 2023
Merged
@stefanprodan stefanprodan modified the milestones: GA, Bootstrap GA Jul 4, 2023
@darkowlzz darkowlzz mentioned this pull request Jul 21, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@makkes makkes makkes left review comments

@hiddeco hiddeco hiddeco left review comments

@relu relu relu approved these changes

@pjbgf pjbgf pjbgf approved these changes

@somtochiama somtochiama somtochiama approved these changes

+1 more reviewer

@darkowlzz darkowlzz darkowlzz approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area/alerting Alerting related issues and PRs area/docs Documentation related issues and pull requests area/receiver Webhook receiver related issues and PRs enhancement New feature or request

Projects

Maintainers' Focus
Archived in project

Milestone

Bootstrap GA

8 participants

@stefanprodan @pjbgf @makkes @darkowlzz @relu @hiddeco @somtochiama