refactor(tibuild-v2): refactor API paths and improve endpoint structure #247
Conversation
|
I Skip it since the diff size(822294 bytes > 80000 bytes) is too large |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @wuhuizuo, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request focuses on refactoring the API paths and improving the endpoint structure for the tibuild-v2 service. The primary goal is to introduce a consistent /api/v2 prefix for all API endpoints and to streamline the structure of the artifact and devbuild services. This involves modifying the design file, client-side code, server-side code, and CLI configurations to reflect the new API structure.
Highlights
- API Path Refactoring: Introduced the
/api/v2prefix for all API paths to maintain consistency across the API. - Service Endpoint Restructuring: Refactored the artifact and devbuild service endpoints to align with the new API path structure, including changes to request/response encoding/decoding and path definitions.
- Payload Simplification: Simplified the
SyncImagepayload by removing the nestedImageSyncRequestand directly usingImageSyncRequestas the payload. - CLI Tool Updates: Updated the CLI tool to reflect the changes in API paths and payload structures, ensuring that the CLI commands are compatible with the refactored API.
- List Devbuild Pagination: Refactor List devbuild to support pagination, including page number, page size, sort and direction.
Changelog
Click here to see the changelog
- experiments/tibuild-v2/design/design.go
- Added
/api/v2prefix to all API paths. - Added
/artifactpath to the artifact service. - Added
/devbuildspath to the devbuild service. - Modified the
syncImageendpoint to useImageSyncRequestdirectly as the payload. - Updated the
devbuildservice methods (list, create, get, update, rerun) to use relative paths within the/devbuildspath. - Added
pageSizeattribute to theListmethod payload.
- Added
- experiments/tibuild-v2/gen/artifact/client.go
- Modified the
SyncImagefunction to acceptImageSyncRequestdirectly as the payload.
- Modified the
- experiments/tibuild-v2/gen/artifact/endpoints.go
- Updated the
NewSyncImageEndpointfunction to useImageSyncRequestas the request type.
- Updated the
- experiments/tibuild-v2/gen/artifact/service.go
- Updated the
SyncImagemethod signature to useImageSyncRequestas the payload type. - Removed the
SyncImagePayloadtype. - Moved
ImageSyncRequestto be the payload type of the artifact service syncImage method.
- Updated the
- experiments/tibuild-v2/gen/devbuild/service.go
- Updated the
Listmethod description to include pagination support. - Added
PerPage,Sort,Direction,PageSizeattributes to theListPayloadtype.
- Updated the
- experiments/tibuild-v2/gen/http/artifact/client/cli.go
- Modified the
BuildSyncImagePayloadfunction to constructImageSyncRequestdirectly from the request body. - Updated the example JSON in the error message to reflect the new payload structure.
- Modified the
- experiments/tibuild-v2/gen/http/artifact/client/encode_decode.go
- Updated the
EncodeSyncImageRequestfunction to useImageSyncRequestas the payload type. - Removed the
marshalArtifactImageSyncRequestToImageSyncRequestRequestBodyandmarshalImageSyncRequestRequestBodyToArtifactImageSyncRequestfunctions.
- Updated the
- experiments/tibuild-v2/gen/http/artifact/client/paths.go
- Updated the
SyncImageArtifactPathfunction to include the/api/v2prefix.
- Updated the
- experiments/tibuild-v2/gen/http/artifact/client/types.go
- Modified the
SyncImageRequestBodytype to includeSourceandTargetdirectly. - Removed the
ImageSyncRequestRequestBodytype. - Updated the
NewSyncImageRequestBodyfunction to construct the request body directly fromImageSyncRequest.
- Modified the
- experiments/tibuild-v2/gen/http/artifact/server/encode_decode.go
- Updated the
DecodeSyncImageRequestfunction to constructImageSyncRequestdirectly from the request body. - Removed the
unmarshalImageSyncRequestRequestBodyToArtifactImageSyncRequestfunction.
- Updated the
- experiments/tibuild-v2/gen/http/artifact/server/paths.go
- Updated the
SyncImageArtifactPathfunction to include the/api/v2prefix.
- Updated the
- experiments/tibuild-v2/gen/http/artifact/server/server.go
- Updated the mount paths for the
SyncImagehandler to include the/api/v2prefix.
- Updated the mount paths for the
- experiments/tibuild-v2/gen/http/artifact/server/types.go
- Modified the
SyncImageRequestBodytype to includeSourceandTargetdirectly. - Removed the
ImageSyncRequestRequestBodytype. - Updated the
NewSyncImageResponseBodyfunction to construct the response body directly fromImageSyncRequest. - Updated the
NewSyncImageImageSyncRequestfunction to construct the payload directly from the request body. - Updated the
ValidateSyncImageRequestBodyfunction to validateSourceandTargetdirectly.
- Modified the
- experiments/tibuild-v2/gen/http/cli/tibuild/cli.go
- Updated the example JSON in the
artifact sync-imagecommand usage to reflect the new payload structure. - Updated the
devbuild listcommand usage to include pagination parameters. - Updated the example JSON in the
devbuild createcommand usage. - Updated the example JSON in the
devbuild updatecommand usage.
- Updated the example JSON in the
- experiments/tibuild-v2/gen/http/devbuild/client/cli.go
- Modified the
BuildListPayloadfunction to construct the payload from the request body and query parameters. - Updated the
BuildCreatePayloadfunction to construct the payload from the request body. - Updated the
BuildUpdatePayloadfunction to construct the payload from the request body.
- Modified the
- experiments/tibuild-v2/gen/http/devbuild/client/encode_decode.go
- Updated the
EncodeListRequestfunction to include pagination parameters in the query string. - Updated the
EncodeListRequestfunction to encode the request body.
- Updated the
- experiments/tibuild-v2/gen/http/devbuild/client/paths.go
- Updated the paths for all devbuild service endpoints to include the
/api/v2prefix.
- Updated the paths for all devbuild service endpoints to include the
- experiments/tibuild-v2/gen/http/devbuild/client/types.go
- Added
ListRequestBodytype. - Updated the
NewListRequestBodyfunction to construct the request body.
- Added
- experiments/tibuild-v2/gen/http/devbuild/server/encode_decode.go
- Updated the
DecodeListRequestfunction to decode the request body and query parameters. - Updated the
NewListPayloadfunction to construct the payload from the request body and query parameters.
- Updated the
- experiments/tibuild-v2/gen/http/devbuild/server/paths.go
- Updated the paths for all devbuild service endpoints to include the
/api/v2prefix.
- Updated the paths for all devbuild service endpoints to include the
- experiments/tibuild-v2/gen/http/devbuild/server/server.go
- Updated the mount paths for all devbuild service handlers to include the
/api/v2prefix.
- Updated the mount paths for all devbuild service handlers to include the
- experiments/tibuild-v2/gen/http/devbuild/server/types.go
- Added
ListRequestBodytype. - Updated the
NewListPayloadfunction to construct the payload from the request body and query parameters. - Added
ValidateListRequestBodyfunction.
- Added
- experiments/tibuild-v2/gen/http/openapi.json
- Updated the OpenAPI specification to reflect the changes in API paths and payload structures.
- experiments/tibuild-v2/pkg/impl/artifact.go
- Updated the
SyncImagemethod signature to useImageSyncRequestas the payload type.
- Updated the
- experiments/tibuild-v2/pkg/impl/devbuild.go
- Updated the
Listmethod description to include pagination support.
- Updated the
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
In the realm of APIs, a path we tread,
Refactoring endpoints, a task ahead.
With prefixes and structures, we bring order and grace,
Ensuring consistency in every space.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
The pull request refactors the API paths and improves the endpoint structure, enhancing the overall organization and maintainability of the API. The changes are well-structured and address the intended improvements effectively.
Summary of Findings
- API Path Structure: The API paths have been refactored to include a version prefix and improve endpoint structure. This change aims to provide better organization and maintainability for the API.
- Payload Structure for syncImage: The payload structure for the
syncImagemethod has been simplified by directly usingImageSyncRequestinstead of wrapping it inSyncImagePayload. This reduces unnecessary nesting and improves code clarity. - List Payload Parameters: The parameters for the
Listmethod have been updated to support pagination and sorting, providing more flexibility in retrieving devbuilds.
Merge Readiness
The code changes are well-structured and address the intended improvements effectively. I would recommend addressing the high severity issue before merging. I am unable to directly approve the pull request, and users should have others review and approve this code before merging.
| return nil, fmt.Errorf("invalid JSON for body, \nerror: %s, \nexample of valid JSON:\n%s", err, "'{\n \"source\": \"Culpa possimus.\",\n \"target\": \"Perferendis nisi non quia debitis.\"\n }'") | ||
| } |
There was a problem hiding this comment.
The error message example still contains ImageSyncRequest which is no longer part of the request body. This should be updated to reflect the new structure.
Style Guide References
| return nil, fmt.Errorf("invalid JSON for body, \nerror: %s, \nexample of valid JSON:\n%s", err, "'{\n \"source\": \"Culpa possimus.\",\n \"target\": \"Perferendis nisi non quia debitis.\"\n }'") | |
| } | |
| return nil, fmt.Errorf("invalid JSON for body, \nerror: %s, \nexample of valid JSON:\n%s", err, "'{\n \"source\": \"Culpa possimus.\",\n \"target\": \"Perferendis nisi non quia debitis.\"\n }'") |
| Description("Sync hotfix image to dockerhub") | ||
| Payload(func() { | ||
| Attribute("ImageSyncRequest", ImageSyncRequest, "Image sync to public, only hotfix is accepted right now") | ||
| Required("ImageSyncRequest") | ||
| }) | ||
| Payload(ImageSyncRequest) |
There was a problem hiding this comment.
Consider adding a comment explaining why the payload is directly ImageSyncRequest instead of ImageSyncRequest within a payload attribute. This can improve readability and understanding for future developers.
Style Guide References
| Description("Sync hotfix image to dockerhub") | |
| Payload(func() { | |
| Attribute("ImageSyncRequest", ImageSyncRequest, "Image sync to public, only hotfix is accepted right now") | |
| Required("ImageSyncRequest") | |
| }) | |
| Payload(ImageSyncRequest) | |
| // Payload(ImageSyncRequest) - Image sync to public, only hotfix is accepted right now | |
| Payload(ImageSyncRequest) |
| return os.Args[0] + ` artifact sync-image --body '{ | ||
| "ImageSyncRequest": { | ||
| "source": "Culpa possimus.", | ||
| "target": "Perferendis nisi non quia debitis." | ||
| } | ||
| "source": "Culpa possimus.", | ||
| "target": "Perferendis nisi non quia debitis." | ||
| }'` + "\n" + | ||
| os.Args[0] + ` devbuild list --size 9048917590767144830 --offset 2393528839850969203 --hotfix false --created-by "Fugiat voluptatibus quia."` + "\n" + | ||
| os.Args[0] + ` devbuild list --body '{ |
There was a problem hiding this comment.
The example should be updated to reflect the new payload structure for artifact sync-image.
Style Guide References
| return os.Args[0] + ` artifact sync-image --body '{ | |
| "ImageSyncRequest": { | |
| "source": "Culpa possimus.", | |
| "target": "Perferendis nisi non quia debitis." | |
| } | |
| "source": "Culpa possimus.", | |
| "target": "Perferendis nisi non quia debitis." | |
| }'` + "\n" + | |
| os.Args[0] + ` devbuild list --size 9048917590767144830 --offset 2393528839850969203 --hotfix false --created-by "Fugiat voluptatibus quia."` + "\n" + | |
| os.Args[0] + ` devbuild list --body '{ | |
| return os.Args[0] + ` artifact sync-image --body '{ | |
| "source": "Culpa possimus.", | |
| "target": "Perferendis nisi non quia debitis." | |
| }'` + "\n" + |
| %[1]s artifact sync-image --body '{ | ||
| "ImageSyncRequest": { | ||
| "source": "Culpa possimus.", | ||
| "target": "Perferendis nisi non quia debitis." | ||
| } | ||
| "source": "Culpa possimus.", | ||
| "target": "Perferendis nisi non quia debitis." |
There was a problem hiding this comment.
The example should be updated to reflect the new payload structure for artifact sync-image.
Style Guide References
| %[1]s artifact sync-image --body '{ | |
| "ImageSyncRequest": { | |
| "source": "Culpa possimus.", | |
| "target": "Perferendis nisi non quia debitis." | |
| } | |
| "source": "Culpa possimus.", | |
| "target": "Perferendis nisi non quia debitis." | |
| %[1]s artifact sync-image --body '{ | |
| "source": "Culpa possimus.", | |
| "target": "Perferendis nisi non quia debitis." | |
| }' |
| err = json.Unmarshal([]byte(devbuildListBody), &body) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("invalid JSON for body, \nerror: %s, \nexample of valid JSON:\n%s", err, "'{\n \"direction\": \"desc\",\n \"per_page\": 9048917590767144830,\n \"sort\": \"updatedAt\"\n }'") |
There was a problem hiding this comment.
The error message example should be updated to reflect the new structure for the list payload.
Style Guide References
| err = json.Unmarshal([]byte(devbuildListBody), &body) | |
| if err != nil { | |
| return nil, fmt.Errorf("invalid JSON for body, \nerror: %s, \nexample of valid JSON:\n%s", err, "'{\n \"direction\": \"desc\",\n \"per_page\": 9048917590767144830,\n \"sort\": \"updatedAt\"\n }'") | |
| return nil, fmt.Errorf("invalid JSON for body, \nerror: %s, \nexample of valid JSON:\n%s", err, "'{\n \"direction\": \"desc\",\n \"per_page\": 9048917590767144830,\n \"sort\": \"updatedAt\"\n }'") |
| if body.Sort != nil { | ||
| if !(*body.Sort == "createdAt" || *body.Sort == "updatedAt") { | ||
| err = goa.MergeErrors(err, goa.InvalidEnumValueError("body.sort", *body.Sort, []any{"createdAt", "updatedAt"})) | ||
| } | ||
| } | ||
| if body.Direction != nil { | ||
| if !(*body.Direction == "asc" || *body.Direction == "desc") { | ||
| err = goa.MergeErrors(err, goa.InvalidEnumValueError("body.direction", *body.Direction, []any{"asc", "desc"})) | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider adding comments to explain the purpose and validation logic for the Sort and Direction fields.
// ValidateListRequestBody runs the validations defined on ListRequestBody
func ValidateListRequestBody(body *ListRequestBody) (err error) {
// Validate Sort field
if body.Sort != nil {
if !(*body.Sort == "createdAt" || *body.Sort == "updatedAt") {
err = goa.MergeErrors(err, goa.InvalidEnumValueError("body.sort", *body.Sort, []any{"createdAt", "updatedAt"}))
}
}
// Validate Direction field
if body.Direction != nil {
if !(*body.Direction == "asc" || *body.Direction == "desc") {
err = goa.MergeErrors(err, goa.InvalidEnumValueError("body.direction", *body.Direction, []any{"asc", "desc"}))
}
}
return
}Style Guide References
|
@gemini-code-assist[bot]: changing LGTM is restricted to collaborators In response to this:
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-sigs/prow repository. |
|
I Skip it since the diff size(834406 bytes > 80000 bytes) is too large |
wuhuizuo
force-pushed
the
feature/tibuild-v2
branch
from
7ed6abe to
394059f
Compare
|
I Skip it since the diff size(836825 bytes > 80000 bytes) is too large |
|
/wip I want to make the sync-image api be a async mode. |
|
I Skip it since the diff size(1514241 bytes > 80000 bytes) is too large |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: wuhuizuo 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 |
No description provided.