Skip to content
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

feat(tibuild): fill result col for devbuild list api #139

Merged
merged 6 commits into from
May 21, 2024

Conversation

wuhuizuo
Copy link
Contributor

@wuhuizuo wuhuizuo commented May 21, 2024

Close #140

Also support filter by creator (createdBy).

Signed-off-by: wuhuizuo [email protected]

Also support filter by creator (`createdBy`).

Signed-off-by: wuhuizuo <[email protected]>
Copy link

ti-chi-bot bot commented May 21, 2024

I have already done a preliminary review for you, and I hope to help you do a better job.

The key changes of this pull request are:

  1. It has upgraded the version of go.uber.org/zap from v1.22.0 to v1.24.0.
  2. It has made some changes in tibuild/Makefile to format and initialize swagger.
  3. It has updated the swagger documentation for the APIs related to tibuild.
  4. It has added a createdBy filter in the List function of DevBuildHandler.
  5. It has made some changes to the formatting of the comments in multiple files.
  6. It has added a check for the createdBy filter in List function of DevBuildRepo.
  7. It has added createdBy as a field in DevBuildListOption.

Potential problems:

  1. If filippo.io/edwards25519 v1.1.0 // indirect and go.uber.org/zap v1.24.0 have breaking changes, it might break the current implementation.
  2. The createdBy filter is not checked for null or empty strings.
  3. There are no unit tests included in this PR to validate these changes.

Fixing suggestions:

  1. Check the changelogs of the upgraded dependencies and verify if there are any breaking changes.
  2. Add checks for createdBy filter, it should not proceed if createdBy is null or an empty string.
  3. Write unit tests to cover these changes and ensure they work as expected.

@ti-chi-bot ti-chi-bot bot added the size/L label May 21, 2024
Copy link

ti-chi-bot bot commented May 21, 2024

I have already done a preliminary review for you, and I hope to help you do a better job.

The pull request, titled "feat(tibuild): fill result col for devbuild list api", contains changes that mainly involve modifying the functionality of the 'tibuild' feature.

Key Changes:

  1. In the Go dependencies, the 'filippo.io/edwards25519' library has been moved, and the version of 'go.uber.org/zap' library has been upgraded from v1.22.0 to v1.24.0.
  2. In the Makefile, the 'swag' command has been updated to include formatting and initialization.
  3. Updates in the 'docs' directory, which includes changes in 'docs.go', 'swagger.json', and 'swagger.yaml', that suggest changes in the API documentation for the endpoint which were likely made by the 'swag' tool.
  4. Updates in the 'controller' directory in 'artifact_helper_handler.go', 'dev_build_handler.go', and 'hotfix_handler.go' files. These changes appear to be primary formatting and alignment changes to Swagger documentation comments with no change in functionality.
  5. In 'dev_build_repo.go', additional filtering by 'CreatedBy' has been added to the 'List' function.
  6. In 'model.go', 'CreatedBy' has been added as an option in 'DevBuildListOption' struct.

Potential Problems:

  1. The 'filippo.io/edwards25519' library has been moved in the 'go.mod' file. If other modules depend on the old location of this library, it may cause problems.
  2. Upgrading the 'go.uber.org/zap' library from v1.22.0 to v1.24.0 can potentially introduce breaking changes if the application was using features that have been modified or removed in the newer version.
  3. 'CreatedBy' as a filter in 'DevBuildListOption' struct has been introduced, but it's not clear if the application will always have this information or if there are any null handling considerations.

Fixing Suggestions:

  1. Ensure that the 'go.uber.org/zap' library's newer version is compatible with your application.
  2. Add null handling or default value for 'CreatedBy' in 'DevBuildListOption' struct, if necessary.
  3. Make sure to thoroughly test the 'List' function in 'dev_build_repo.go' after adding the new 'CreatedBy' filter.
  4. Confirm that moving the 'filippo.io/edwards25519' library does not impact other areas of the application.

Signed-off-by: wuhuizuo <[email protected]>
Copy link

ti-chi-bot bot commented May 21, 2024

I have already done a preliminary review for you, and I hope to help you do a better job.

The pull request titled "feat(tibuild): fill result col for devbuild list api" mainly includes the following changes:

  1. A new filter createdBy is added to the DevBuildListOption struct and the corresponding List method in DevBuildRepo. This allows users to filter devbuilds by the creator.

  2. The Go modules dependencies are updated in go.mod and go.sum files. For instance, go.uber.org/zap is updated from version v1.22.0 to v1.24.0.

  3. The Makefile for tibuild is updated to install swag if it's not installed and then run swag fmt and swag init.

  4. Some formatting changes are made in the Swagger and API comments to improve readability.

  5. The docs.go, swagger.json and swagger.yaml files under tibuild/docs are updated to reflect the added createdBy filter.

Potential problems:

  1. There's no test case provided for the newly added createdBy filter. This might not ensure the functionality works as expected.

  2. In the List method of DevBuildRepo, the error returned is always ErrInternalError without distinguishing different types of errors.

  3. The error messages returned by the updated functions do not provide much context about the source of the error.

  4. The update of go.uber.org/zap to new version is not justified. There may be a need to check if this update won't break anything.

Suggestions:

  1. Add test cases for the new createdBy filter to ensure it works as expected.

  2. Differentiate the types of errors in the List method of DevBuildRepo for better error handling.

  3. Improve the error messages to provide more context about the source of the error.

  4. Ensure the updated version of go.uber.org/zap does not break any existing functionalities.

Copy link

ti-chi-bot bot commented May 21, 2024

I have already done a preliminary review for you, and I hope to help you do a better job.

This pull request introduces a few changes:

  1. It updates the dependencies in go.mod and go.sum files. The go.uber.org/zap package is updated from v1.22.0 to v1.24.0 and the filippo.io/edwards25519 package is moved from a standalone require statement to the group require statement. There's no removal or addition of new dependencies.

  2. It updates the Makefile for the target swagger to check if swag is installed and if not, install it.

  3. Updates the Swagger documentation of the API endpoints in artifact_helper_handler.go, dev_build_handler.go and hotfix_handler.go files. It seems like the changes are mainly formatting, making the annotations more readable.

  4. It adds a new filter parameter createdBy to the ListDevbuild function in dev_build_handler.go. This allows the API to filter builds by the creator.

  5. It modifies the DevBuildListOption struct in model.go to include the createdBy field, reflecting the change in point 4.

  6. There are changes in the dev_build_repo.go and dev_build_repo_test.go files to support the new createdBy filter in the database queries.

  7. It also modifies some test files, like cloud_event_client_test.go and dev_build_service_test.go, to adapt to these changes.

Potential Issues:

  1. Dependency updates can sometimes introduce breaking changes. Make sure the new package versions are backward-compatible with the existing codebase or the codebase is updated to adapt to the changes in the updated packages.

  2. The createdBy filtering in the list API could lead to performance issues if the database is not properly indexed.

  3. Changes in the Swagger documentation should be carefully reviewed to ensure the API's behavior is correctly described.

Suggested Fixes:

  1. Make sure to thoroughly test the application after updating the dependencies to identify any potential compatibility issues.

  2. If the createdBy field is frequently used for filtering, consider adding an index in the database to speed up these queries.

  3. Review the Swagger documentation changes to ensure they are accurate. It might be beneficial to generate and manually inspect the rendered Swagger UI for any irregularities.

  4. Update the unit tests to consider the newly added createdBy filter.

Copy link

@purelind purelind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

Copy link

ti-chi-bot bot commented May 21, 2024

[LGTM Timeline notifier]

Timeline:

  • 2024-05-21 06:29:04.240498485 +0000 UTC m=+2153097.997634058: ☑️ agreed by purelind.

@wuhuizuo
Copy link
Contributor Author

/approve

Copy link

ti-chi-bot bot commented May 21, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: purelind, 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot bot added the approved label May 21, 2024
@ti-chi-bot ti-chi-bot bot merged commit bb0e065 into main May 21, 2024
2 of 3 checks passed
@ti-chi-bot ti-chi-bot bot deleted the feature/add-report-status-cols-in-list-results branch May 21, 2024 07:04
wuhuizuo added a commit to PingCAP-QE/ee-ops that referenced this pull request May 21, 2024
ti-chi-bot bot pushed a commit to PingCAP-QE/ee-ops that referenced this pull request May 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[tibuild] support filter by user and get the artifacts in list.
2 participants