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

Add support for extension APIService definition updates #36

Merged
merged 4 commits into from
Jan 25, 2022

Conversation

adammw
Copy link
Contributor

@adammw adammw commented Aug 4, 2021

Add support for extension APIService definition updates via the cert-controller library.

The APIService definition points to a service and caBundle like the existing webhook types, and extends the kubernetes API server using the aggregation layer to provide access to alternate apiGroups via that server.

There were no tests for other types of WebhookInfo apart from the Validating type - however I can work on adding some if required.

@adammw adammw force-pushed the adammw/api-service branch from cb4bd77 to d629957 Compare August 4, 2021 00:40
@adammw
Copy link
Contributor Author

adammw commented Aug 4, 2021

cc @mmirecki @adrianludwin @ritazh for visibility/review

@maxsmythe
Copy link
Contributor

Thanks for the PR! Code LGTM.

If you are willing to add the tests, it would definitely be helpful.

@mmirecki
Copy link
Contributor

mmirecki commented Aug 4, 2021

/lgtm

@adammw
Copy link
Contributor Author

adammw commented Dec 9, 2021

Adding tests for the other webhook types in #37, will rebase onto that once merged.

Copy link
Member

@ritazh ritazh left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @adammw!
PR LGTM
Pending testings rebase from #37

Copy link
Member

@sozercan sozercan left a comment

Choose a reason for hiding this comment

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

lgtm

@maxsmythe
Copy link
Contributor

#37 merged

@ritazh
Copy link
Member

ritazh commented Jan 24, 2022

@adammw now that #37 has been merged, please address the merge conflict and add tests for APIService.

@adammw adammw force-pushed the adammw/api-service branch from c7dc1e2 to 6fdd60c Compare January 25, 2022 01:17
Signed-off-by: Adam Malcontenti-Wilson <amalcontenti-wilson@zendesk.com>
@adammw adammw force-pushed the adammw/api-service branch from 6fdd60c to 6b04f6e Compare January 25, 2022 01:17
Signed-off-by: Adam Malcontenti-Wilson <amalcontenti-wilson@zendesk.com>
Signed-off-by: Adam Malcontenti-Wilson <amalcontenti-wilson@zendesk.com>
Signed-off-by: Adam Malcontenti-Wilson <amalcontenti-wilson@zendesk.com>
@adammw adammw force-pushed the adammw/api-service branch from f6c5cb1 to ce78882 Compare January 25, 2022 01:22
@codecov-commenter
Copy link

Codecov Report

Merging #36 (ce78882) into master (cd3703c) will increase coverage by 0.29%.
The diff coverage is 45.45%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #36      +/-   ##
==========================================
+ Coverage   53.35%   53.64%   +0.29%     
==========================================
  Files           1        1              
  Lines         373      384      +11     
==========================================
+ Hits          199      206       +7     
- Misses        115      117       +2     
- Partials       59       61       +2     
Flag Coverage Δ
unittests 53.64% <45.45%> (+0.29%) ⬆️

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

Impacted Files Coverage Δ
pkg/rotator/rotator.go 53.64% <45.45%> (+0.29%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cd3703c...ce78882. Read the comment docs.

@adammw
Copy link
Contributor Author

adammw commented Jan 25, 2022

Rebased and resolved conflicts. Tests require version bumps to latest (k8s libs -> 0.23, controller-runtime -> 0.11, go -> 1.17) otherwise it gets stuck in version dependency errors.

Copy link
Contributor

@maxsmythe maxsmythe left a comment

Choose a reason for hiding this comment

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

LGTM

@ritazh @sozercan I'll leave it to you to merge if the rebase LGTY

@ritazh
Copy link
Member

ritazh commented Jan 25, 2022

Thanks @adammw!

@ritazh ritazh merged commit 54af894 into open-policy-agent:master Jan 25, 2022
@adammw adammw deleted the adammw/api-service branch January 25, 2022 04:02
@sozercan
Copy link
Member

This might causes issues with GK when we need to vendor if it requires controller-runtime 0.11.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants