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

refactor(apps/prod/tekton/configs): rename pipeline from pingcap-build-package to pingcap-build-package-linux #1377

Closed
wants to merge 5 commits into from

Conversation

wuhuizuo
Copy link
Collaborator

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

@ti-chi-bot ti-chi-bot bot requested a review from purelind December 13, 2024 09:00
@ti-chi-bot ti-chi-bot bot added the area/apps label Dec 13, 2024
Copy link
Contributor

ti-chi-bot bot commented Dec 13, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from wuhuizuo, ensuring that each of them provides their approval before proceeding. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found 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 env/prod will deploy on the main product cluster size/XL labels Dec 13, 2024
Copy link
Contributor

ti-chi-bot bot commented Dec 13, 2024

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

The pull request primarily involves changes related to renaming a pipeline and its related files, and removing files related to the darwin-v2 version of the pipeline.

Key changes:

  1. The pipeline pingcap-build-package is renamed to pingcap-build-package-linux.
  2. File pingcap-build-package-darwin-v2.yaml and associated files are deleted.
  3. Several files have been updated to reflect the new pipeline name.
  4. The pipeline files for 'darwin' have been updated, with some tasks removed and others added.

Potential Problems:

  1. The deletion of the pingcap-build-package-darwin-v2.yaml file and related files might cause issues if there are dependencies on these files in other places of the project.
  2. There's no description about why the darwin-v2 version is removed. If it's still in use or will be used in the future, this could be a problem.
  3. The renaming of the pipeline will also require updates wherever this pipeline is referenced. If any references are missed, it could lead to errors.
  4. Some files do not have a newline at the end, this might cause problems for some tools which expect or require them.
  5. The PR lacks a detailed description and context, making it hard for reviewers to understand the purpose and impact of the changes.

Fixing suggestions:

  1. Ensure that the deletion of the pingcap-build-package-darwin-v2.yaml and related files does not have any adverse effects on other parts of the project.
  2. Update all references to the renamed pipeline across the project.
  3. Add a newline at the end of the files that are missing one.
  4. Provide a detailed description in the PR about the changes made and their impact on the project.
  5. Conduct thorough testing to ensure that the changes do not introduce any new issues or regressions.

Copy link
Contributor

ti-chi-bot bot commented Dec 13, 2024

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

The Pull Request (PR) primarily involves renaming and refactoring a pipeline and some tasks in a Tekton configuration. Here are the key changes:

  1. The pipeline pingcap-build-package is renamed to pingcap-build-package-linux.

  2. The task pingcap-build-binaries-darwin-v2.yaml is renamed to pingcap-build-binaries-linux.yaml.

  3. The task pingcap-build-binaries.yaml is deleted.

  4. Updates to the pingcap-build-package-darwin.yaml file, including changes to the task parameters and some additions to the results.

  5. The pingcap-build-package-darwin-v2.yaml is deleted.

  6. The kustomization.yaml in both pipelines and tasks is updated to reflect the above changes.

Potential Problems:

  1. The PR lacks a detailed description. It only contains the name of the author, which does not provide enough context about the changes.

  2. If other scripts or services refer to the renamed files or tasks, those references would break due to these changes.

  3. By deleting the pingcap-build-binaries.yaml task, it would cause failures if there are other dependencies or pipelines using this task.

  4. The renamed pipeline and task are specified for Linux (-linux suffix), but there is no similar renaming or corresponding configuration for other platforms like Windows or MacOS.

Suggestions:

  1. Add a detailed description to the PR explaining the changes and their impact.

  2. Ensure all references to the renamed files or tasks are updated accordingly.

  3. Before deleting any task, check for dependencies and ensure they won't be affected, or make necessary adjustments.

  4. If the renaming is intended to distinguish platform-specific configurations, ensure similar naming and configurations for other platforms are also considered and implemented.

…pipeline be same with linux pipeline (#1376)

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

Signed-off-by: wuhuizuo <wuhuizuo@126.com>
…arwin-v2

the refactor pipeline is stable runned for a week, let's simply its name:
- delete the old one `pingcap-build-package-darwin`
- rename from `pingcap-build-package-darwin-v2` to `pingcap-build-package-darwin`.

Signed-off-by: wuhuizuo <wuhuizuo@126.com>
…inaries-darwin-v2 to pingcap-build-binaries-darwin

Signed-off-by: wuhuizuo <wuhuizuo@126.com>
…ld-package` to `pingcap-build-package-linux`

Signed-off-by: wuhuizuo <wuhuizuo@126.com>
Signed-off-by: wuhuizuo <wuhuizuo@126.com>
@wuhuizuo wuhuizuo force-pushed the feature/dev-build-2 branch from cab3339 to 82cb598 Compare December 18, 2024 08:41
Copy link
Contributor

ti-chi-bot bot commented Dec 18, 2024

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

The pull request primarily includes renaming and refactoring the pipelines and tasks related to the pingcap-build-package in the Tekton configurations. Here are the key changes:

  1. The pipeline pingcap-build-package and the task pingcap-build-binaries are renamed to pingcap-build-package-linux and pingcap-build-binaries-linux respectively.
  2. The pipeline pingcap-build-package-darwin-v2 and the corresponding task are deleted.
  3. The file pingcap-build-package-darwin.yaml is updated with changes in the names of parameters and tasks.
  4. The kustomization.yaml files are updated to include the renamed pipelines and tasks.

Potential problems:

  1. All references to the renamed pipelines and tasks need to be updated as well. This seems to have been done in the pull request, but it would be necessary to double-check all Tekton configurations and possibly application code to ensure nothing is missed.
  2. Deleting the pingcap-build-package-darwin-v2 pipeline and task may impact any processes dependent on them. If there are any, they need to be updated or redirected to use the renamed pipeline and task.
  3. The renaming is done to distinguish between Linux and Darwin builds. However, the differentiation isn't clear in the naming of tasks and pipelines for Darwin builds. This could potentially lead to confusion in the future.
  4. The pingcap-build-binaries-darwin-v2.yaml is renamed to pingcap-build-binaries-linux.yaml but the task spec inside doesn't seem to have been updated to reflect this change.

Fixing suggestions:

  1. Ensure all references to the renamed pipelines and tasks are updated.
  2. Verify if any processes are dependent on the deleted pipeline and task, and update them as necessary.
  3. Consider renaming the tasks and pipelines for Darwin builds to include 'darwin' in their names for consistency and clarity.
  4. Update the task spec of pingcap-build-binaries-linux.yaml to match its filename. The task spec still references "darwin" which seems to be a mistake.

@wuhuizuo wuhuizuo closed this Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/apps env/prod will deploy on the main product cluster size/XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant