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

build: rollback building with centos7 edition images #520

Merged
merged 4 commits into from
Jan 6, 2025

Conversation

wuhuizuo
Copy link
Contributor

@wuhuizuo wuhuizuo commented Jan 6, 2025

No description provided.

- add versions to be checked.
- add image check step.

Signed-off-by: wuhuizuo <[email protected]>
rollout the builder image to centos7 edition for compatibility.

Signed-off-by: wuhuizuo <[email protected]>
bump tiup version to v1.16.1 for composing `ctl` package since v8.4.0

Signed-off-by: wuhuizuo <[email protected]>
@ti-chi-bot ti-chi-bot bot requested a review from purelind January 6, 2025 04:27
Copy link

ti-chi-bot bot commented Jan 6, 2025

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

The pull request titled build: rollback building with centos7 edition images is a rollback operation that involves reverting to CentOS 7 edition images for building various components and packages. The major changes in this pull request can be summarized as follows:

  1. A new function check_image_existed was added to the ci.sh script. This function checks if a given image exists. It will be helpful in ensuring that the necessary images are available during the build process.

  2. Multiple versions of different components and packages were updated to include newer versions (v9.0.0 and v8.5.0).

  3. The builder images for various components and packages have been rolled back to CentOS 7 edition images.

  4. The URL for downloading TiUP has been updated to version v1.16.1.

Potential problems and fixing suggestions:

  1. There does not seem to be any immediate errors or problems with the changes in this pull request. However, you need to ensure that the necessary CentOS 7 edition images are available for the build process. If they are not available, the build process may fail.

  2. Given the rollback nature of this pull request, it is crucial to test the build process thoroughly to ensure that there are no compatibility issues with the CentOS 7 edition images. This could include running comprehensive unit and integration tests.

  3. It would be a good idea to add comments to the check_image_existed function, describing what it does, what arguments it expects, and what it returns. This will make the code more readable and maintainable.

  4. Since there is a version update for multiple components, make sure to validate these new versions and check if they are compatible with other components in the system.

@ti-chi-bot ti-chi-bot bot added the size/M label Jan 6, 2025
Copy link

ti-chi-bot bot commented Jan 6, 2025

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

Summary of Key Changes:

  1. A new function check_image_existed has been introduced to check if a docker image exists.
  2. The version lists in several functions have been updated to include v9.0.0 and v8.5.0.
  3. The image used for building in the script packages.yaml.tmpl has been changed in multiple places.
  4. The version of gomplate in pull-verify-packages-config.yaml has been updated from v4.1.0 to v4.3.0.
  5. The crane tool has been added to the installation steps in the pull-verify-packages-config.yaml workflow.
  6. The URL to download the tiup has been updated from v1.16.0 to v1.16.1.

Potential Problems:

  1. The existence of the docker images is not guaranteed. It relies on external resources which might not always be available.
  2. The versions have been hardcoded which might require frequent updates.
  3. The changes in the builder image might cause inconsistencies or incompatibilities with the existing code or environment.
  4. The URL to download the tiup is also hardcoded, and it might be unavailable in the future.

Suggestions for Improvement:

  1. It would be better to handle the scenario when the docker image does not exist more gracefully.
  2. Consider using a more dynamic approach for versioning, possibly from a configuration file or environment variables.
  3. Ensure the new builder images are compatible with the existing code and environment.
  4. It would be better to handle the scenario when the tiup download URL is not available.

@wuhuizuo
Copy link
Contributor Author

wuhuizuo commented Jan 6, 2025

/approve

Copy link

ti-chi-bot bot commented Jan 6, 2025

[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 Jan 6, 2025
@ti-chi-bot ti-chi-bot bot merged commit 27bb519 into main Jan 6, 2025
3 checks passed
@ti-chi-bot ti-chi-bot bot deleted the fix/build-with-centos7 branch January 6, 2025 05:43
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