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.

Sign up for GitHub

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

Jump to bottom

fix: catch route failure and trigger retry #149

Merged
merged 24 commits into from
Jan 6, 2021
Merged

fix: catch route failure and trigger retry #149

merged 24 commits into from
Jan 6, 2021

Conversation

gxthrj
Copy link
Contributor

@gxthrj gxthrj commented Dec 30, 2020

Fix: #148

@codecov-io
Copy link

codecov-io commented Dec 30, 2020 •
edited
Loading

Codecov Report

Merging #149 (de46782) into master (782730a) will decrease coverage by 1.67%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #149      +/-   ##
==========================================
- Coverage   36.27%   34.60%   -1.68%     
==========================================
  Files          28       28              
  Lines        1257     1312      +55     
==========================================
- Hits          456      454       -2     
- Misses        771      828      +57     
  Partials       30       30
Impacted Files Coverage Δ
pkg/seven/apisix/route.go 31.73% <0.00%> (ø)
pkg/seven/state/builder.go 0.00% <0.00%> (ø)
pkg/seven/state/route_worker.go 0.00% <0.00%> (ø)
pkg/seven/state/service_worker.go 0.00% <0.00%> (ø)
pkg/seven/state/solver.go 3.50% <0.00%> (-6.25%) ⬇️

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 782730a...de46782. Read the comment docs.

@gxthrj gxthrj marked this pull request as ready for review January 4, 2021 01:01
tokers
tokers reviewed Jan 4, 2021
View reviewed changes
pkg/seven/state/builder.go Outdated Show resolved Hide resolved
pkg/seven/state/builder.go Outdated Show resolved Hide resolved
pkg/seven/state/builder.go Outdated Show resolved Hide resolved
pkg/seven/state/builder.go Outdated Show resolved Hide resolved
pkg/seven/state/solver.go Outdated
}

func WaitWorkerGroup(id string, resultChan chan CRDStatus) (string, error) {
for {
Copy link
Contributor

Choose a reason for hiding this comment

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

Your code can be simplified to one line, since the select only has one arm.

r := <-resultChan

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

test/e2e/ingress/route.go Outdated Show resolved Hide resolved
test/e2e/scaffold/crd.go Show resolved Hide resolved
test/e2e/scaffold/httpbin.go Outdated Show resolved Hide resolved
test/e2e/scaffold/httpbin.go Show resolved Hide resolved
test/e2e/scaffold/ingress.go Outdated Show resolved Hide resolved
@gxthrj gxthrj requested a review from tokers January 4, 2021 14:20
tokers
tokers reviewed Jan 5, 2021
View reviewed changes
pkg/seven/state/builder.go Show resolved Hide resolved
pkg/seven/apisix/route.go Outdated Show resolved Hide resolved
pkg/seven/state/builder.go Outdated Show resolved Hide resolved
pkg/seven/state/service_worker.go Show resolved Hide resolved
pkg/seven/state/service_worker.go Outdated Show resolved Hide resolved
pkg/seven/utils/types.go Outdated Show resolved Hide resolved
test/e2e/go.mod Outdated Show resolved Hide resolved
test/e2e/scaffold/httpbin.go Outdated Show resolved Hide resolved
test/e2e/scaffold/ingress.go Outdated Show resolved Hide resolved
test/e2e/scaffold/scaffold.go Show resolved Hide resolved
tokers
tokers requested changes Jan 5, 2021
View reviewed changes
Copy link
Contributor

@tokers tokers left a comment

Choose a reason for hiding this comment

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

Make sure the e2e case can run.

@gxthrj gxthrj requested a review from tokers January 5, 2021 10:53
@gxthrj
Copy link
Contributor Author

gxthrj commented Jan 5, 2021

Make sure the e2e case can run.

image

lilien1010
lilien1010 approved these changes Jan 6, 2021
View reviewed changes
Copy link
Member

@lilien1010 lilien1010 left a comment

Choose a reason for hiding this comment

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

LGTM

@tokers tokers merged commit 2a8e8a7 into apache:master Jan 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lilien1010 lilien1010 lilien1010 approved these changes

@tokers tokers tokers approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support getting the error of route creating/updating and trigger the retry correctly
4 participants
@gxthrj @codecov-io @lilien1010 @tokers