-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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: Fix missing response types in Swagger API endpoints #7611
Conversation
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@@ -785,7 +785,7 @@ func (b *BaseApi) GetDirConfig(c *gin.Context) { | |||
// @Summary Get default html | |||
// @Accept json | |||
// @Param type path string true "type" | |||
// @Success 200 {object} response.FileInfo | |||
// @Success 200 {object} response.WebsiteHtmlRes | |||
// @Security ApiKeyAuth | |||
// @Security Timestamp | |||
// @Router /websites/default/html/{type} [get] |
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.
The code changes look generally appropriate and follow standard API documentation conventions, but there are some slight inconsistencies:
-
Status Code Mismatch: The
@Success
annotations for methods that return arrays or objects (likeList website names
,Get site permissions
, etc.) should use{array}
instead of juststring
. For instance:// @Success 200 {array} response.WebsiteOption
-
Response Objects: Some responses might not explicitly define their response structure using types like
response.NginxRewriteRes
,response.WebsiteProxyConfig
etc. While it's generally okay to omit defining a specific type if it's straightforward and clear what data is expected, it can be beneficial to define custom structs to ensure consistency and clarity. -
File Info Object: Replace
{object}
with{object} response.FileInfo
in the/websites/default/html/{type}
method sinceFileInfo
represents file information which needs a more precise response definition.
These adjustments will improve readability and maintainability of your API endpoint definitions.
@@ -786,7 +786,7 @@ func (b *BaseApi) Keys(c *gin.Context) { | |||
// @Tags File | |||
// @Summary Read file by Line | |||
// @Param request body request.FileReadByLineReq true "request" | |||
// @Success 200 | |||
// @Success 200 {object} response.FileLineContent | |||
// @Security ApiKeyAuth | |||
// @Security Timestamp | |||
// @Router /files/read [post] |
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.
- The first line of the
ListFiles
function should be changed to use a struct literal instead of casting.
files, err := fileService.GetFiles(req)
2.helper.ErrorWithDetail(c, constant.CodeErrInternalServer, constant.ErrTypeInternalServer, err)
can be simplified to helper.FailWithDetail(c, "internal server error", err)
err = fileService.GetFileList(req)
if err != nil {
helper.FailWithDetail(c, "failed to get file list", err)
return
}
helper.SuccessWithData(c, fileList)
-
The change from using
constant.ErrTypeInternalServer
to just stating"internal server error"
is more explicit and cleaner. -
In the
Keys
function, if there are multiple keys that need to be returned, it might be better to have those values stored in an array or map before passing them toSuccessWithData
.
In summary, these changes streamline some boilerplate code such as logging errors or formatting responses and make them easier to read and maintain.
@@ -161,7 +161,7 @@ func (b *BaseApi) GetProcess(c *gin.Context) { | |||
// @Summary Get Supervisor process config | |||
// @Accept json | |||
// @Param request body request.SupervisorProcessFileReq true "request" | |||
// @Success 200 | |||
// @Success 200 {string} content | |||
// @Security ApiKeyAuth | |||
// @Security Timestamp | |||
// @Router /host/tool/supervisor/process/file [post] |
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 are no irregularities or significant issues with this Go/Gin API code snippet based on the provided knowledge cutoff of September 1, 2021. However, here are some minor observations:
- Comment Update: The comment block for
/host/tool/supervisor/process
should reflect that it serves "Supervisor process config" rather than just beingtrue
.
// @Summary Get Supervisor process config
// @Desc Retrieve the configuration details for Supervisor processes.
This change ensures clarity about what this endpoint actually accomplishes.
-
Response Type for
/host/tool/log
:- Currently,
/host/tool/log
uses"logContent"
in its@Success
tag, which is not consistent. It might be more appropriate to return the actual log content, not just a placeholder string. For example:["error", "warning", "info"]
- Currently,
-
Consistency Across Endpoints: Ensure that all endpoints use uniform JSON responses unless they have specific data formats. This can make them easier to consume across different client applications.
-
Logging: Ensure proper logging throughout the endpoints if required for debugging or monitoring purposes.
-
Error Handling: Consider implementing better error handling practices, especially around invalid input data or unexpected errors during API operations.
These points, while not breaking any fundamental aspects of the code, could improve readability, consistency, and maintainability.
Quality Gate passedIssues Measures |
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
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: wanghe-fit2cloud 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 |
Refs #7607