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

feat: add webhooks for consumer/tls/upstream #667

Merged
merged 9 commits into from
Oct 8, 2021
Merged

Conversation

fgksgf
Copy link
Member

@fgksgf fgksgf commented Sep 6, 2021

Please answer these questions before submitting a pull request

  • Why submit this pull request?

  • Bugfix

  • New feature provided

  • Improve performance

  • Backport patches

  • Related issues


New feature or improvement

  • Describe the details and related test reports.

Add webhooks for consumer/tls/upstream.

TODO

  • add webhooks
  • add e2e test cases
  • update helm chart

@codecov-commenter
Copy link

codecov-commenter commented Sep 6, 2021

Codecov Report

Merging #667 (c0f5291) into master (6a8658d) will decrease coverage by 0.25%.
The diff coverage is 28.42%.

❗ Current head c0f5291 differs from pull request most recent head d0b1f9e. Consider uploading reports for the commit d0b1f9e to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #667      +/-   ##
==========================================
- Coverage   34.77%   34.51%   -0.26%     
==========================================
  Files          60       63       +3     
  Lines        5916     6078     +162     
==========================================
+ Hits         2057     2098      +41     
- Misses       3608     3729     +121     
  Partials      251      251              
Impacted Files Coverage Δ
pkg/api/validation/apisix_consumer.go 0.00% <0.00%> (ø)
pkg/api/validation/apisix_tls.go 0.00% <0.00%> (ø)
pkg/api/validation/apisix_upstream.go 0.00% <0.00%> (ø)
pkg/apisix/apisix.go 67.50% <ø> (ø)
pkg/apisix/nonexistentclient.go 41.79% <0.00%> (-0.64%) ⬇️
pkg/ingress/apisix_consumer.go 0.00% <0.00%> (ø)
pkg/ingress/apisix_tls.go 0.00% <0.00%> (ø)
pkg/ingress/controller.go 1.06% <0.00%> (-0.01%) ⬇️
pkg/ingress/endpoint.go 0.00% <0.00%> (ø)
pkg/ingress/endpointslice.go 0.00% <0.00%> (ø)
... and 10 more

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 6a8658d...d0b1f9e. Read the comment docs.

@tao12345666333 tao12345666333 added this to the 1.3.0 milestone Sep 7, 2021
@tao12345666333 tao12345666333 added changelog Issues with this label should be added to changelog when public a new release enhancement New feature or request labels Sep 7, 2021
pkg/api/validation/apisix_consumer.go Outdated Show resolved Hide resolved
pkg/api/validation/apisix_consumer.go Show resolved Hide resolved
pkg/api/validation/apisix_consumer.go Show resolved Hide resolved
pkg/api/validation/apisix_consumer.go Outdated Show resolved Hide resolved
@fgksgf
Copy link
Member Author

fgksgf commented Sep 21, 2021

I just found that there is already basic validation in CRDs, so could you provide some cases that can not be validated in CRD, then I will replace the logic in webhooks with yours.

@tao12345666333
Copy link
Member

@fgksgf you can just fix the lint error. then we can move forward.

@fgksgf
Copy link
Member Author

fgksgf commented Sep 23, 2021

@fgksgf you can just fix the lint error. then we can move forward.

I just didn't figure out why the linter report the error:
image
image

There is no lint errors when I run make lint locally.

@tao12345666333
Copy link
Member

@fgksgf I think, it's a GitHub Action's bug.

I have trigerd it on my fork using same env. It can be pass. https://github.com/tao12345666333/ingress-controller/runs/3684766821?check_suite_focus=true

@tao12345666333
Copy link
Member

We can skip the lint error.
WDYT? @gxthrj @tokers

@tokers
Copy link
Contributor

tokers commented Sep 23, 2021

We can skip the lint error.
WDYT? @gxthrj @tokers

Agree, we already have our own lints.

@gxthrj
Copy link
Contributor

gxthrj commented Sep 30, 2021

Yes, I have checked the lint too, it can pass.

If there is no other need to modify, we can merge this PR. cc @tao12345666333 @tokers @lingsamuel

@lingsamuel
Copy link
Member

+1 can't repro. I triggered it again, let's see if it will pass. If not, we may need to create an ignore rule.

@tao12345666333
Copy link
Member

+1

@tao12345666333
Copy link
Member

ApisixRoute v2beta2 version was introduced in #698
The corresponding check was not covered in this PR.

Let's merge this PR first, and then increase the coverage of v2beta2 in subsequent PRs.

@tao12345666333
Copy link
Member

Added a new issue for tracking #703

@fgksgf fgksgf deleted the webhook branch October 8, 2021 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog Issues with this label should be added to changelog when public a new release enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants