kubewarden / kubewarden-controller

Manage admission policies in your Kubernetes cluster with ease
https://kubewarden.io
Apache License 2.0
182 stars 30 forks source link

Update controller code to use the controller-runtime helper function to create or update resources #730

Closed jvanz closed 1 month ago

jvanz commented 2 months ago

Description

This PR updates the controller code responsible to update the policy server resources to use the controller-runtime CreateOrUpdate helper function to abstract the logic of detecting if the resource should be created or updated. Making the code more simple and easy to maintain.

During this process the unit tests testing the removed code have been removed as well. Which bring us to the spin off PR of this one, the integration tests reorganization #731 . The old tests have been reorganized to describe better the context of what the tests are validating. As well as updating the whole policy server controller tests to be independent as suggested by the Ginkgo documentation. After that, to ensure that the controller continue to works as before a bunch of integrations tests have been added covering the removed unit tests

~NOTE: I've not added test to cover the metrics configurations yet. I'll add a future PR to cover that. I do not want to wait more in this PR to get some feedback from the team.~ This is not necessary anymore. I've added tests for the metrics and tracing configuration here as well.

Fix https://github.com/kubewarden/kubewarden-controller/issues/723

Test

make test

Additional information

codecov[bot] commented 2 months ago

Codecov Report

Attention: Patch coverage is 84.98024% with 38 lines in your changes are missing coverage. Please review.

Project coverage is 73.84%. Comparing base (66fac8e) to head (5adaba0).

Files Patch % Lines
internal/pkg/admission/policy-server-deployment.go 77.27% 28 Missing and 7 partials :warning:
internal/pkg/admission/policy-server-service.go 91.66% 1 Missing and 1 partial :warning:
internal/pkg/admission/policy-server-configmap.go 94.73% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #730 +/- ## ========================================== + Coverage 71.02% 73.84% +2.81% ========================================== Files 28 28 Lines 2071 1908 -163 ========================================== - Hits 1471 1409 -62 + Misses 459 381 -78 + Partials 141 118 -23 ``` | [Flag](https://app.codecov.io/gh/kubewarden/kubewarden-controller/pull/730/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubewarden) | Coverage Δ | | |---|---|---| | [integration-tests](https://app.codecov.io/gh/kubewarden/kubewarden-controller/pull/730/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubewarden) | `62.36% <81.81%> (+2.88%)` | :arrow_up: | | [unit-tests](https://app.codecov.io/gh/kubewarden/kubewarden-controller/pull/730/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubewarden) | `46.07% <41.89%> (+1.31%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubewarden#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

jvanz commented 2 months ago

@kubewarden/kubewarden-developers I take this opportunity to verify some of the policy controller flaky test that we can see in the main branch. I could hit this test failing regularly in this branch. Therefore, I've started by reorganizing the tests to be independent. This way I can run the problematic test individually. That's why, you can see another test reorganization in the policy controller tests. ;)

jvanz commented 2 months ago

@kubewarden/kubewarden-developers I've split this PR into two. The second one, #731, has the test reorganization that I did while working on this change. As the tests added in this PR require the changes on #731, this PR is based on that changes. Thus, #731 should be merge before.

Moving this PR to block until we merge #731. Once it's merged, I'll rebase this on top of main again and we will have a cleaner diff.

jvanz commented 2 months ago

@fabriziosestito I've addressed your comments. Can you review it again? Thanks!