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: retries field to support zero value #2298

Merged
merged 4 commits into from
Feb 1, 2022
Merged

Conversation

Chaunceyan
Copy link
Contributor

fix issue #2297 by changing retries type int to *int

Please answer these questions before submitting a pull request, or your PR will get closed.

Why submit this pull request?

  • Bugfix
  • New feature provided
  • Improve performance
  • Backport patches

What changes will this PR take into?

Change the type of the field api/internal/core/entity/entity.go:166 retries from int to *int.

Related issues

fix/resolve #0001

Checklist:

  • Did you explain what problem does this PR solve? Or what new features have been added?
  • Have you added corresponding test cases?
  • Have you modified the corresponding document?
  • Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

fix issue apache#2297 by changing retries type int to *int
@Chaunceyan Chaunceyan closed this Jan 27, 2022
add unit test to the modified field
@Chaunceyan Chaunceyan reopened this Jan 27, 2022
@codecov-commenter
Copy link

codecov-commenter commented Jan 27, 2022

Codecov Report

Merging #2298 (3b6779d) into master (ff84e03) will not change coverage.
The diff coverage is n/a.

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

@@           Coverage Diff           @@
##           master    #2298   +/-   ##
=======================================
  Coverage   68.16%   68.16%           
=======================================
  Files         127      127           
  Lines        3374     3374           
  Branches      830      830           
=======================================
  Hits         2300     2300           
  Misses       1074     1074           
Flag Coverage Δ
frontend-e2e-test 68.16% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out 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 ff84e03...6b5cd97. Read the comment docs.

@juzhiyuan
Copy link
Member

Thanks! Let's cc @zaunist @Baoyuantop to have a check

@zaunist
Copy link
Contributor

zaunist commented Jan 27, 2022

Better add some E2E test case to cover this change.

@juzhiyuan
Copy link
Member

Hi @zaunist, maybe @Chaunceyan is not familiar with this, could you please give some hints or steps?

@Chaunceyan
Copy link
Contributor Author

Better add some E2E test case to cover this change.

Hi @zaunist, no problem, I suppose some cases on the route handler will do?

@zaunist
Copy link
Contributor

zaunist commented Jan 28, 2022

Better add some E2E test case to cover this change.

Hi @zaunist, no problem, I suppose some cases on the route handler will do?

I think it's better to add cases to https://github.com/apache/apisix-dashboard/blob/master/api/test/e2enew/upstream/upstream_retry.go

@zaunist
Copy link
Contributor

zaunist commented Jan 28, 2022

You can just create a upstream with zero value, and check it return values.

add e2e test for retries field
@Chaunceyan
Copy link
Contributor Author

You can just create a upstream with zero value, and check it return values.

Sure, added in the new commit

Copy link
Contributor

@zaunist zaunist left a comment

Choose a reason for hiding this comment

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

LGTM

@Chaunceyan
Copy link
Contributor Author

@zaunist @juzhiyuan It seems some of the ginkgo e2e tests failed. Is this a concern or they were being broken?

@juzhiyuan
Copy link
Member

I just retriggered the CI, wait for a moment.

@Chaunceyan
Copy link
Contributor Author

@juzhiyuan Hi Zhiyuan, happy new year. I guess I found the problem. My e2e test case introduced some side effects in the etcd which leads to the other failed cases. Would you mind trigger the CI again when available? Thanks

@juzhiyuan
Copy link
Member

Sure! Just rerun

@zaunist
Copy link
Contributor

zaunist commented Jan 31, 2022

Sure! Just rerun

666

@lijing-21
Copy link

Hi @Chaunceyan, thank you for your contribution!

Here is the Contributor T-shirt form[1], if you're interested, kindly take a look :)

[1] https://github.com/apache/apisix/blob/master/CONTRIBUTING.md#contributor-t-shirt

@Chaunceyan
Copy link
Contributor Author

Hi @Chaunceyan, thank you for your contribution!

Here is the Contributor T-shirt form[1], if you're interested, kindly take a look :)

[1] https://github.com/apache/apisix/blob/master/CONTRIBUTING.md#contributor-t-shirt

Thanks, I filled the form. Cheers

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.

7 participants