-
Notifications
You must be signed in to change notification settings - Fork 721
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
placement: add API support #1894
Conversation
Signed-off-by: disksing <i@disksing.com>
Signed-off-by: disksing <i@disksing.com>
Codecov Report
@@ Coverage Diff @@
## master #1894 +/- ##
==========================================
- Coverage 78.08% 77.32% -0.76%
==========================================
Files 163 164 +1
Lines 16338 16468 +130
==========================================
- Hits 12757 12734 -23
- Misses 2583 2725 +142
- Partials 998 1009 +11
Continue to review full report at Codecov.
|
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. do we need some tests to test these api?
@lhy1024 I think we need the test. But I don't have enough time to add it now.. |
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.
rest LGTM
server/api/rule.go
Outdated
return | ||
} | ||
if !cluster.IsPlacementRulesEnabled() { | ||
h.rd.JSON(w, http.StatusPreconditionFailed, "placement rules feature is disabled") |
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.
How about defining an 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.
Good idea.
Signed-off-by: disksing <i@disksing.com>
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.
I think we could introduce the Middleware to reduce the duplicated code.
In ideal, we should define two middlewares, one for bootstrapping, another for placement rules. However, the bootstrapping middleware has no relationship with this PR, maybe we can combine these middlewares into single one first, and leave the TODOs to future refactoring or as PCP problem.
if !cluster.IsPlacementRulesEnabled() { | ||
h.rd.JSON(w, http.StatusPreconditionFailed, placementDisabledErr.Error()) | ||
return | ||
} |
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.
It is really verbose of these lines of code. Maybe you want to try the middleware https://github.com/gorilla/mux#middleware
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.
I'll see what I can do for now.
Signed-off-by: disksing <i@disksing.com> Co-Authored-By: Shafreeck Sea <shafreeck@users.noreply.github.com>
Signed-off-by: disksing <i@disksing.com> Co-Authored-By: Shafreeck Sea <shafreeck@users.noreply.github.com>
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.
/run-all-tests |
What problem does this PR solve?
continue merge placement rules.
What is changed and how it works?
Check List
Tests
Code changes