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

ci: add job "Pull test for packages cfg" #167

Merged

Conversation

wuhuizuo
Copy link
Contributor

Signed-off-by: wuhuizuo wuhuizuo@126.com

Signed-off-by: wuhuizuo <wuhuizuo@126.com>
@ti-chi-bot ti-chi-bot bot requested review from jayl1e and purelind December 17, 2023 08:43
Copy link

ti-chi-bot bot commented Dec 17, 2023

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

Review for Pull Request: ci: add job "Pull test for packages cfg"

Summary

The pull request is adding a new workflow job named "Pull test for packages cfg" to the project. This job is triggered by the pull_request event on the main branch, and it will run some steps to test the package builder, generate package artifacts, and generate package images.

Potential Problems and Suggestions

  1. The pull_request event is triggered for any new pull request opened against the main branch, even if the changes are not related to the packages directory. A better approach would be to trigger this job only when there are changes in the packages directory. Therefore, we can narrow down the path filters to include only the packages directory as follows:
on:
  pull_request:
    branches:
      - main
    paths:
      - "packages/**"
  1. The pull request is not providing enough details about why this job is necessary and what problem it is trying to solve. Adding more context in the pull request description can help reviewers understand the motivation behind this change.

  2. The pull_request event is triggered only for new pull requests, not for changes to existing pull requests. If the changes made to an existing pull request modify the packages directory, this job will not be triggered. It might be useful to consider adding a similar job to other events, such as push, to ensure that the package tests are run on every change.

Conclusion

Overall, this pull request is a good addition to the project, and it can help to ensure that the package builder and related scripts are working as expected. However, it needs some improvements to make it more targeted and informative.

@ti-chi-bot ti-chi-bot bot added the size/M label Dec 17, 2023
@wuhuizuo
Copy link
Contributor Author

/approve

Copy link

ti-chi-bot bot commented Dec 17, 2023

[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 /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 Dec 17, 2023
@ti-chi-bot ti-chi-bot bot merged commit 75a0f13 into fix/refactor-slow-tiflash-builder-flow Dec 17, 2023
1 check passed
@ti-chi-bot ti-chi-bot bot deleted the ci/verify-packages-configs branch December 17, 2023 08:48
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.

1 participant