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: ApisixRoute update operations is not effective #319

Merged
merged 3 commits into from
Mar 31, 2021

Conversation

tokers
Copy link
Contributor

@tokers tokers commented Mar 29, 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
    bug: Modify the route.yaml configuration to not update apisix synchronously #312

  • What does this PR do:

    • Unify the route, upstream synchronization to the manifest, it's used by ApisixRouteController, IngressController and EndpointController.
    • Fixed the route synchronization bug.
    • Remove useless codes in pkg/seven/state.
    • Modify the e2e case Create and remove the ApisixRoute, adding update operation and sending requests to APISIX.

@tokers tokers added the bug Something isn't working label Mar 29, 2021
@tokers tokers requested a review from gxthrj March 29, 2021 07:43
@tokers
Copy link
Contributor Author

tokers commented Mar 29, 2021

@gxthrj Please take a look when you have time.

name: httpbin-route
spec:
http:
- name: rule1
Copy link
Contributor

Choose a reason for hiding this comment

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

should add cases :
the diff name in the same ApisixRoute.
the same name in the diff ApisixRoute.

Copy link
Contributor Author

@tokers tokers Mar 29, 2021

Choose a reason for hiding this comment

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

Cannot totally capture your mean.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean add test cases:

  1. In the same ApisixRoute yaml ,but change spec.http.name when update
  2. In the different ApisixRoute yaml, but have the same spec.http.name, which means name is conflict.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, will add it.

@codecov-io
Copy link

Codecov Report

Merging #319 (b747891) into master (8277995) will increase coverage by 2.59%.
The diff coverage is 30.07%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #319      +/-   ##
==========================================
+ Coverage   39.25%   41.84%   +2.59%     
==========================================
  Files          42       39       -3     
  Lines        3577     3444     -133     
==========================================
+ Hits         1404     1441      +37     
+ Misses       2012     1842     -170     
  Partials      161      161              
Impacted Files Coverage Δ
pkg/ingress/controller/apisix_route.go 0.00% <0.00%> (ø)
pkg/ingress/controller/endpoint.go 0.00% <0.00%> (ø)
pkg/ingress/controller/ingress.go 7.87% <0.00%> (-0.90%) ⬇️
pkg/kube/translation/apisix_route.go 30.19% <0.00%> (-0.16%) ⬇️
pkg/ingress/controller/manifest.go 67.79% <67.79%> (ø)
test/e2e/e2e.go

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 8277995...b747891. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants