Skip to content
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

fix: Handle lack of CRDs gracefully for every controller #3013

Merged
merged 37 commits into from
Oct 13, 2022
Merged

fix: Handle lack of CRDs gracefully for every controller #3013

merged 37 commits into from
Oct 13, 2022

Conversation

czeslavo
Copy link
Contributor

@czeslavo czeslavo commented Oct 3, 2022 •
edited
Loading

What this PR does / why we need it:

  • simplifies controllers definitions by removing AutoHandler, leaving only Enabled flag
  • makes handling of CRDs missing installation common for all controllers that require CRDs (in case CRDs are missing, we log it and disable the controller without crashing)
  • adds missing unit tests
  • adds e2e test covering an issue with crashing controller container when CRDs are missing

Which issue this PR fixes:

Fixes #3009

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

  • the CHANGELOG.md release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR

@czeslavo czeslavo temporarily deployed to Configure ci October 3, 2022 18:34 Inactive
@czeslavo czeslavo temporarily deployed to Configure ci October 3, 2022 18:39 Inactive
@czeslavo czeslavo marked this pull request as ready for review October 3, 2022 18:43
@czeslavo czeslavo requested a review from a team as a code owner October 3, 2022 18:43
@czeslavo czeslavo temporarily deployed to Configure ci October 3, 2022 18:43 Inactive
@czeslavo czeslavo added the area/maintenance Cleanup, refactoring, and other maintenance improvements that don't change functionality. label Oct 3, 2022
@czeslavo czeslavo temporarily deployed to Configure ci October 3, 2022 19:00 Inactive
@czeslavo czeslavo temporarily deployed to Configure ci October 3, 2022 19:23 Inactive
pmalek
pmalek requested changes Oct 4, 2022
View reviewed changes
Copy link
Member

@pmalek pmalek left a comment

Choose a reason for hiding this comment

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

Some nits and suggestions but overall we're on good tracks.

test/e2e/features_test.go Outdated Show resolved Hide resolved
test/e2e/features_test.go Outdated Show resolved Hide resolved
test/e2e/features_test.go Outdated Show resolved Hide resolved
test/e2e/features_test.go Outdated Show resolved Hide resolved
internal/manager/conditions_crd_test.go Outdated Show resolved Hide resolved
internal/manager/conditions_crd_test.go Outdated Show resolved Hide resolved
internal/manager/conditions_ingressapi.go Outdated Show resolved Hide resolved
internal/manager/conditions_ingressapi_test.go Outdated Show resolved Hide resolved
internal/manager/conditions_ingressapi_test.go Outdated Show resolved Hide resolved
internal/manager/conditions_ingressapi_test.go Outdated Show resolved Hide resolved
@czeslavo @pmalek
Update test/e2e/features_test.go
7f2a67c 
Co-authored-by: Patryk Małek <patryk.malek@konghq.com>
@czeslavo czeslavo temporarily deployed to Configure ci October 4, 2022 09:16 Inactive
@czeslavo @pmalek
Update internal/manager/conditions_ingressapi.go
e0a1cc1 
Co-authored-by: Patryk Małek <patryk.malek@konghq.com>
@czeslavo czeslavo temporarily deployed to Configure ci October 4, 2022 09:17 Inactive
@czeslavo czeslavo temporarily deployed to Configure ci October 4, 2022 09:26 Inactive
@czeslavo czeslavo temporarily deployed to Configure ci October 4, 2022 09:48 Inactive
@czeslavo czeslavo temporarily deployed to Configure ci October 13, 2022 09:01 Inactive
@czeslavo czeslavo force-pushed the controllers-conditional-enablement branch from 1591c44 to 073b572 Compare October 13, 2022 09:06
@czeslavo czeslavo temporarily deployed to Configure ci October 13, 2022 09:06 Inactive
@czeslavo czeslavo force-pushed the controllers-conditional-enablement branch from 073b572 to 9080e0f Compare October 13, 2022 09:07
@czeslavo czeslavo temporarily deployed to Configure ci October 13, 2022 09:07 Inactive
randmonkey
randmonkey previously approved these changes Oct 13, 2022
View reviewed changes
@czeslavo czeslavo temporarily deployed to Configure ci October 13, 2022 09:32 Inactive
@czeslavo czeslavo temporarily deployed to Configure ci October 13, 2022 09:44 Inactive
jrsmroz
jrsmroz previously approved these changes Oct 13, 2022
View reviewed changes
Copy link
Contributor

@jrsmroz jrsmroz left a comment •
edited
Loading

Choose a reason for hiding this comment

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

stamp <approved>

pmalek
pmalek reviewed Oct 13, 2022
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
internal/manager/conditions.go Show resolved Hide resolved
@czeslavo @pmalek
Update CHANGELOG.md
890e647 
Co-authored-by: Patryk Małek <patryk.malek@konghq.com>
@czeslavo czeslavo dismissed stale reviews from jrsmroz and randmonkey via 890e647 October 13, 2022 10:02
@czeslavo czeslavo temporarily deployed to Configure ci October 13, 2022 10:02 Inactive
@czeslavo czeslavo temporarily deployed to Configure ci October 13, 2022 10:07 Inactive
@czeslavo czeslavo requested a review from pmalek October 13, 2022 10:08
pmalek
pmalek approved these changes Oct 13, 2022
View reviewed changes
Copy link
Member

@pmalek pmalek left a comment

Choose a reason for hiding this comment

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

This looks 👍 overall. One small nitpick is that, ideally we'd try to make the PRs a bit smaller so that they are easier to digest by the reviewers ;)

@czeslavo czeslavo temporarily deployed to Configure ci October 13, 2022 10:31 Inactive
@czeslavo czeslavo temporarily deployed to Configure ci October 13, 2022 10:31 Inactive
@czeslavo czeslavo dismissed mlavacca’s stale review October 13, 2022 10:52

All suggestions have been addressed.

@czeslavo czeslavo merged commit eb47a69 into main Oct 13, 2022
@czeslavo czeslavo deleted the controllers-conditional-enablement branch October 13, 2022 10:54
@czeslavo czeslavo mentioned this pull request Oct 13, 2022
Merged
1 task
@rainest rainest mentioned this pull request Jun 26, 2023
Closed
1 task
@gallolp gallolp mentioned this pull request Sep 4, 2023
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pmalek pmalek pmalek approved these changes

@jrsmroz jrsmroz jrsmroz approved these changes

@randmonkey randmonkey randmonkey left review comments

@mlavacca mlavacca Awaiting requested review from mlavacca

Assignees

@czeslavo czeslavo

Labels
area/maintenance Cleanup, refactoring, and other maintenance improvements that don't change functionality.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Handle lack of CRDs gracefully for every controller
5 participants
@czeslavo @pmalek @jrsmroz @randmonkey @mlavacca