-
-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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
refactor: provide API for change plugin motion status synchronously #4745
Conversation
Codecov Report
@@ Coverage Diff @@
## main #4745 +/- ##
============================================
- Coverage 61.58% 61.50% -0.08%
- Complexity 2797 2798 +1
============================================
Files 403 403
Lines 14725 14770 +45
Branches 1023 1028 +5
============================================
+ Hits 9068 9085 +17
- Misses 5165 5193 +28
Partials 492 492
|
ready for review |
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
@halo-dev/sig-halo
private ExpectedAction action; | ||
private boolean async; | ||
|
||
enum ExpectedAction { |
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.
不建议在这里制造一种新概念。整个系统尽量保持一种风格。
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.
没懂 保持一种风格指的是保持什么
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.
参数为 boolean 值?
@@ -201,6 +203,27 @@ public RouterFunction<ServerResponse> endpoint() { | |||
.response(responseBuilder() | |||
.implementation(Plugin.class)) | |||
) | |||
.PUT("plugins/{name}/motion-status", this::changeMotionStatus, |
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.
这里似乎不是一个常规的资源型请求 - PUT resources
。
得确认一下分配插件管理权限后是否能正常调用。
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.
可以的 ,与 PUT plugins/{name}/reload 一样,权限正确
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.
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.
参考:
PUT /api/v1/namespaces/{namespace}/pods/{name}/ephemeralcontainers
PUT /api/v1/namespaces/{namespace}/pods/{name}/proxy
https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.28/#replace-ephemeralcontainers-pod-v1-core
kubernetes 的 api-conventions 中也没有关于 PUT 子资源的描述 https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#verbs-on-resources,但是他们是这么做的
那么对于这种更新部分字段操作的自定义 API 不用 PUT 用什么呢
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.
直接对子资源的某些字段进行 PUT 操作似乎也是合理的。建议我们先完善一下 RFC。
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
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JohnNiang The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind improvement
/area core
/area console
What this PR does / why we need it:
提供允许同步更改插件运行状态的 API
Which issue(s) this PR fixes:
Fixes #4744
Does this PR introduce a user-facing change?