-
Notifications
You must be signed in to change notification settings - Fork 338
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: support resolveGranularity of ApisixRoute #1251
Conversation
return nil | ||
} | ||
|
||
func (u *upstreamService) Create(ctx context.Context, relation *v1.UpstreamServiceRelation) error { | ||
// to do: delete one of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does it mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete one of the upstreams. Currently, only service is deleted. There will be some redundant objects, but the results will not be affected.
Also, please adjust your parameter order
|
Codecov Report
@@ Coverage Diff @@
## master #1251 +/- ##
==========================================
+ Coverage 39.77% 40.92% +1.15%
==========================================
Files 75 75
Lines 6967 6954 -13
==========================================
+ Hits 2771 2846 +75
+ Misses 3897 3795 -102
- Partials 299 313 +14
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Done. |
I want you can add an e2e test case to cover #870, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
thanks
if err != nil { | ||
if err == apisixcache.ErrNotFound { | ||
continue | ||
update := func(upsName string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can it be a function instead of a local variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but it has many contexts, requires more parameters, and can't be reused. I do not recommend this.
Type of change:
What this PR does / why we need it:
ref:
fixes:
resolveGranularity
should be considered in upstream id generation #1157Pre-submission checklist: