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(apps/prod/tekton/configs): switch to new darwin build pipeline in trigger templates #1372

Merged
merged 1 commit into from
Dec 10, 2024

Conversation

wuhuizuo
Copy link
Collaborator

switch to pipeline: pingcap-build-package-darwin-v2

Signed-off-by: wuhuizuo [email protected]

@ti-chi-bot ti-chi-bot bot requested a review from purelind December 10, 2024 05:57
@ti-chi-bot ti-chi-bot bot added area/apps env/prod will deploy on the main product cluster labels Dec 10, 2024
Copy link
Contributor

ti-chi-bot bot commented Dec 10, 2024

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

Based on the pull request title and description, the key changes are:

  • The pipeline for building on Darwin platform is switched from pingcap-build-package-darwin to pingcap-build-package-darwin-v2.
  • The os parameter is added to the pipeline configuration, with the value darwin.
  • The boskos-server-url parameter is added to the pipeline configuration, with the value http://boskos.apps.svc.

Potential problems that I can identify are:

  • It is not clear from the pull request description why the pipeline is being switched. Is there a specific problem with the old pipeline that is being addressed with the new one? Is there a performance improvement or other benefit to the new pipeline?
  • The pull request only changes the pipeline configuration for the Darwin platform, but it is not clear if the pipeline is being used for other platforms as well. If so, it is possible that the changes made for the Darwin platform could affect the pipeline for other platforms.
  • The boskos-server-url parameter is added without any explanation of what it is or why it is needed. This could be confusing for other contributors who are not familiar with this project.

Suggestions for fixing these potential problems are:

  • Add a more detailed explanation in the pull request description about why the pipeline is being switched, and what benefits it provides over the old pipeline.
  • Verify that the changes made for the Darwin platform do not have any unintended effects on other platforms, and consider making similar changes to the pipeline configuration for those platforms if necessary.
  • Add a brief explanation or comment about the boskos-server-url parameter, to help other contributors understand why it is needed and what it does.

@ti-chi-bot ti-chi-bot bot added the size/S label Dec 10, 2024
…n trigger templates

switch to pipeline: `pingcap-build-package-darwin-v2`

Signed-off-by: wuhuizuo <[email protected]>
@wuhuizuo wuhuizuo force-pushed the feature/switch-to-new-darwin-build-pipeline branch from c4fd990 to 10125e1 Compare December 10, 2024 06:04
Copy link
Contributor

ti-chi-bot bot commented Dec 10, 2024

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

Based on the pull request title and description, the key change is switching to a new build pipeline named pingcap-build-package-darwin-v2 in the trigger templates for the apps/prod/tekton/configs directory.

From the diff, it seems that the change mainly involves replacing the pipeline name in the pipelineRef section of the YAML configurations for building binaries on MacOS with the new name. Additionally, two new parameters, os and boskos-server-url, have been introduced in the params section of the YAML configurations.

As for potential problems, it's hard to tell from the diff alone. However, it may be worth checking if the new pipeline is compatible with the existing infrastructure, and if the new parameters are being used correctly.

For fixing suggestions, I would recommend checking the compatibility of the new pipeline with the existing infrastructure, and making sure the new parameters are being used correctly and consistently throughout the configurations. Additionally, it's always a good practice to test the changes thoroughly before merging them into the main branch.

@wuhuizuo
Copy link
Collaborator Author

/approve

Copy link
Contributor

ti-chi-bot bot commented Dec 10, 2024

[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 10, 2024
@ti-chi-bot ti-chi-bot bot merged commit 146170a into main Dec 10, 2024
4 checks passed
@ti-chi-bot ti-chi-bot bot deleted the feature/switch-to-new-darwin-build-pipeline branch December 10, 2024 06:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved area/apps env/prod will deploy on the main product cluster size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant