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.

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

fix: use webhook ns if empty, more test versions #568

Merged
merged 2 commits into from
Dec 1, 2023

Conversation

beeme1mr
Copy link
Member

@beeme1mr beeme1mr commented Dec 1, 2023

This PR

Reintroduces using admission webhook namespace if pod namespace is empty

Related Issues

Fixes #500

Notes

This was accidentally removed during a recent refactor. It wasn't caught in our tests because the oldest version that's tested is 1.25. I've added a comment above the change to make it clear why this logic is there.

Signed-off-by: Michael Beemer <beeme1mr@users.noreply.github.com>
@beeme1mr beeme1mr requested a review from a team as a code owner December 1, 2023 14:15
Copy link

codecov bot commented Dec 1, 2023

Codecov Report

Merging #568 (7619de5) into main (4c0fe34) will increase coverage by 0.07%.
The diff coverage is 100.00%.

❗ Current head 7619de5 differs from pull request most recent head a4ffad1. Consider uploading reports for the commit a4ffad1 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #568      +/-   ##
==========================================
+ Coverage   87.90%   87.98%   +0.07%     
==========================================
  Files          10       10              
  Lines         926      932       +6     
==========================================
+ Hits          814      820       +6     
  Misses         89       89              
  Partials       23       23              
Files Coverage Δ
webhooks/pod_webhook.go 86.66% <100.00%> (+0.80%) ⬆️
Flag Coverage Δ
unit-tests 87.98% <100.00%> (+0.07%) ⬆️

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

@toddbaert
Copy link
Member

toddbaert commented Dec 1, 2023

If we're going to maintain code to support older versions, why not just test older versions as well so we actually see when we break them?

@toddbaert toddbaert self-requested a review December 1, 2023 14:21
@toddbaert
Copy link
Member

@beeme1mr @thisthat can we add 1.22 - 1.28 here?

@beeme1mr
Copy link
Member Author

beeme1mr commented Dec 1, 2023

We could update the matrix to [ v1.22.0, v1.28.0 ] so that it covers the minimum tested version and the latest. Testing all the versions in between would lead to painfully long test runs.

@toddbaert
Copy link
Member

toddbaert commented Dec 1, 2023

We could update the matrix to [ v1.22.0, v1.28.0 ] so that it covers the minimum tested version and the latest. Testing all the versions in between would lead to painfully long test runs.

They run in parallel: https://github.com/open-feature/open-feature-operator/actions/runs/7061357527/job/19222819640?pr=569

Signed-off-by: Todd Baert <toddbaert@gmail.com>
@toddbaert toddbaert changed the title fix: reintroduce using admission webhook namespace if pod namespace is empty fix: use webhook ns if empty, more test versions Dec 1, 2023
@beeme1mr beeme1mr merged commit b9b619d into main Dec 1, 2023
16 checks passed
@beeme1mr beeme1mr deleted the fix-webhook-for-older-k8s-version branch December 1, 2023 15:55
@github-actions github-actions bot mentioned this pull request Dec 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants