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: route crd add timeout fields #609

Merged
merged 3 commits into from
Sep 1, 2021
Merged

Conversation

Donghui0
Copy link
Contributor

@Donghui0 Donghui0 commented Jul 30, 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.
    Route.Spec add plugin_config_id And timeout fields.

Backport patches

  • Why need to backport?

  • Source branch

  • Related commits and pull requests

  • Target branch

@codecov-commenter
Copy link

codecov-commenter commented Jul 30, 2021

Codecov Report

Merging #609 (dfbcdc5) into master (7291212) will increase coverage by 0.07%.
The diff coverage is 35.41%.

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

@@            Coverage Diff             @@
##           master     #609      +/-   ##
==========================================
+ Coverage   34.40%   34.47%   +0.07%     
==========================================
  Files          55       57       +2     
  Lines        5468     5734     +266     
==========================================
+ Hits         1881     1977      +96     
- Misses       3362     3510     +148     
- Partials      225      247      +22     
Impacted Files Coverage Δ
pkg/ingress/apisix_cluster_config.go 0.00% <0.00%> (ø)
pkg/ingress/controller.go 1.07% <0.00%> (-0.01%) ⬇️
pkg/ingress/ingress.go 8.29% <0.00%> (-0.12%) ⬇️
pkg/ingress/status.go 0.00% <0.00%> (ø)
pkg/kube/translation/context.go 85.71% <0.00%> (-14.29%) ⬇️
pkg/kube/translation/ingress.go 77.48% <0.00%> (-11.14%) ⬇️
pkg/ingress/manifest.go 53.74% <27.77%> (-9.42%) ⬇️
pkg/apisix/nonexistentclient.go 44.44% <28.57%> (-1.99%) ⬇️
pkg/apisix/cluster.go 27.20% <31.00%> (+1.87%) ⬆️
pkg/kube/translation/apisix_route.go 26.57% <41.66%> (+0.41%) ⬆️
... and 7 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 7291212...7b00277. Read the comment docs.

@@ -99,6 +100,7 @@ type ApisixRouteHTTP struct {
Backends []*ApisixRouteHTTPBackend `json:"backends" yaml:"backends"`
Websocket bool `json:"websocket" yaml:"websocket"`
Plugins []*ApisixRouteHTTPPlugin `json:"plugins,omitempty" yaml:"plugins,omitempty"`
PluginConfigID string `json:"plugin_config_id,omitempty" yaml:"plugin_config_id,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO this is a terrible way to expose the ID of an APISIX configuration in the declarative configuration way, it's not easy for people to configure it.

Another CRD to group a bunch of plugins can be introduced, say ApisixPluginConfig, and here we should specify the name of such CR.

Copy link
Member

@tao12345666333 tao12345666333 left a comment

Choose a reason for hiding this comment

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

you can split into two PR

@tao12345666333
Copy link
Member

@Donghui0

#638

@tao12345666333 tao12345666333 added this to the 1.3.0 milestone Aug 18, 2021
@tao12345666333 tao12345666333 added this to In progress in v1.3 Planning Aug 18, 2021
@Donghui0 Donghui0 changed the title feat: route crd add plugin_config_id/timeout fields feat: route crd add timeout fields Aug 18, 2021
@tao12345666333
Copy link
Member

@Donghui0
Copy link
Contributor Author

@Donghui0 please check the unit-test errors. https://github.com/apache/apisix-ingress-controller/pull/609/checks?check_run_id=3358610706

This error is that the time to run the unit test exceeds the default 10 minutes.
image

Is it due to a remote host configuration problem? For example: When I submit the code, there are other tasks occupying resources that cause resource contention, which causes the unit test task to run out of time ?

There is no error when running make unit-test in my local environment.
image
can you help me solve it ? tanks~

@tao12345666333
Copy link
Member

@Donghui0 yep, I re-run the job,no more errors.

@tao12345666333
Copy link
Member

ping @tokers @gxthrj for review

@gxthrj gxthrj merged commit d4a832c into apache:master Sep 1, 2021
v1.3 Planning automation moved this from In progress to Done Sep 1, 2021
@Donghui0 Donghui0 deleted the feat-route-spec branch April 3, 2023 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants