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

refactor: use helper from controller runtime #725

Merged
merged 4 commits into from
May 21, 2024
Merged

refactor: use helper from controller runtime #725

merged 4 commits into from
May 21, 2024

Conversation

flavio
Copy link
Member

@flavio flavio commented Apr 19, 2024

Use a more concise way to handle "not found" errors that is made possible by using a controller runtime helper.

@flavio flavio requested a review from a team as a code owner April 19, 2024 07:53
@codecov Codecov
Copy link

codecov bot commented Apr 19, 2024 •
edited

Codecov Report

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

Project coverage is 71.78%. Comparing base (9c3dd61) to head (783f2bd).

Files Patch % Lines
internal/pkg/admission/policy-server-ca-secret.go 0.00% 3 Missing ⚠️
...g/admission/policy-server-pod-disruption-budget.go 0.00% 3 Missing ⚠️
controllers/admissionpolicy_controller.go 0.00% 1 Missing ⚠️
internal/pkg/admission/policy-server-configmap.go 0.00% 1 Missing ⚠️
internal/pkg/admission/policy-server-deployment.go 0.00% 1 Missing ⚠️
internal/pkg/admission/policy-server-service.go 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #725      +/-   ##
==========================================
- Coverage   72.84%   71.78%   -1.07%     
==========================================
  Files          28       28              
  Lines        1812     1804       -8     
==========================================
- Hits         1320     1295      -25     
- Misses        373      388      +15     
- Partials      119      121       +2
Flag Coverage Δ
integration-tests 59.42% <45.00%> (-1.29%) ⬇️
unit-tests 50.11% <20.00%> (-0.08%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

viccuad
viccuad requested changes Apr 19, 2024
View reviewed changes
Copy link
Member

@viccuad viccuad left a comment

Choose a reason for hiding this comment

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

golangci-lint complains that the errors are not wrapped, we should tackle this :/.

jvanz
jvanz requested changes Apr 19, 2024
View reviewed changes
Copy link
Member

@jvanz jvanz left a comment

Choose a reason for hiding this comment

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

Besides the linter errors, LGTM

@flavio
Copy link
Member Author

flavio commented Apr 22, 2024

@jvanz, @viccuad : I've updated golanglint-ci and then fixed all the warnings via 8d93633. Please take a look at the commit description to see the reasons of the changes I've made

@flavio
Copy link
Member Author

flavio commented Apr 22, 2024

Note: this cannot be merged until the 1.12 release is done

@jvanz
Copy link
Member

jvanz commented Apr 24, 2024

It seems there is something wrong, because tests and linter are failing. =(

@flavio flavio assigned flavio and unassigned flavio May 3, 2024
@flavio
Copy link
Member Author

flavio commented May 9, 2024

@jvanz I'm a bit stuck with other tasks, would you be so kind to take this over once your PR about the owner reference is merged? 🙏

@jvanz jvanz self-assigned this May 9, 2024
@jvanz jvanz force-pushed the use-client-ignore-not-found branch 3 times, most recently from 12a7c96 to 64fc1ce Compare May 20, 2024 19:47
@jvanz
Copy link
Member

jvanz commented May 20, 2024

@kubewarden/kubewarden-developers , I've updated this PR rebasing it on top of main and did some fixes to make the CI happy. To make all our CI green I had to update the controller-gen binary used. Otherwise, we get a nil pointer deref. That's why you can see so many line changes, I've commited the result of the generator after updating it. Let me know if you're fine with that. Otherwise, I can update the generator in another PR or commit the updated manifest files in another PR.

@jvanz jvanz requested review from viccuad and a team May 20, 2024 19:55
flavio
flavio commented May 21, 2024
View reviewed changes
Copy link
Member Author

@flavio flavio left a comment

Choose a reason for hiding this comment

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

I'm fine with the update of controller-gen binary. Everything looks good to me, I left some minor comments.

controllers/clusteradmissionpolicy_controller_test.go Outdated Show resolved Hide resolved
controllers/clusteradmissionpolicy_controller_test.go Outdated Show resolved Hide resolved
internal/pkg/admission/policy-server-ca-secret_test.go Outdated Show resolved Hide resolved
fabriziosestito
fabriziosestito approved these changes May 21, 2024
View reviewed changes
Copy link
Contributor

@fabriziosestito fabriziosestito left a comment

Choose a reason for hiding this comment

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

LGTM apart from the changes requested

@jvanz jvanz force-pushed the use-client-ignore-not-found branch from 8b992e9 to 9cee72f Compare May 21, 2024 15:17
flavio and others added 4 commits May 21, 2024 12:18
@flavio @jvanz
chore(deps): update golangci-lint
39bbfe4 
Use latest stable release of golangci-lint

Signed-off-by: Flavio Castelli <fcastelli@suse.com>
@jvanz
chore: update controller-gen binary.
0f228c8 
Updates the controller-gen binary used to generate the manifests.

Signed-off-by: José Guilherme Vanz <jguilhermevanz@suse.com>
@flavio @jvanz
refactor: use helper from controller runtime
802e9d1 
Use a more concise way to handle "not found" errors that is made
possible by using a controller runtime helper.

Signed-off-by: Flavio Castelli <fcastelli@suse.com>
@jvanz
fix: update go.sum.
783f2bd 
Updates the go.sum file.

Signed-off-by: José Guilherme Vanz <jguilhermevanz@suse.com>
@jvanz jvanz force-pushed the use-client-ignore-not-found branch from 9cee72f to 783f2bd Compare May 21, 2024 15:22
@jvanz
Copy link
Member

jvanz commented May 21, 2024

@kubewarden/kubewarden-developers I've updated this branch rebasing it on top of main and squashing some commits to reduce the number of them.

@flavio flavio merged commit bfbd965 into kubewarden:main May 21, 2024
7 of 9 checks passed
@flavio flavio deleted the use-client-ignore-not-found branch May 21, 2024 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabriziosestito fabriziosestito fabriziosestito approved these changes

@jvanz jvanz jvanz approved these changes

@viccuad viccuad Awaiting requested review from viccuad

Assignees

@jvanz jvanz

Labels
None yet
Projects
Kubewarden
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@flavio @jvanz @fabriziosestito @viccuad