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: add tools mariadb client 5.5.68 #479

Merged

Conversation

purelind
Copy link
Contributor

@purelind purelind commented Nov 1, 2024

Install mysql client from mariadb v5.5.68.

@ti-chi-bot ti-chi-bot bot requested a review from wuhuizuo November 1, 2024 05:49
Copy link

ti-chi-bot bot commented Nov 1, 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, the key changes are:

  • The installation of MariaDB client version 5.5.68 has been added to the Dockerfile.
  • The environment variable MARIADB_VERSION has been defined to store the MariaDB version.
  • The PATH environment variable has been updated to include the path to the MariaDB client binary.
  • A check has been added to ensure that only the amd64 architecture is supported for installation, and if the architecture is not supported, an error message is displayed.

Regarding potential problems or issues, there are a few things to consider:

  • It's unclear why the specific version of MariaDB client 5.5.68 is being used, and whether it's the most appropriate version for the project's needs.
  • The check for the supported architecture may not account for other architectures that could be used in the future.
  • The removal of the mysql-community-client package installation may cause issues if it was previously being used.

To address these potential issues, you could take the following steps:

  • Confirm whether version 5.5.68 of MariaDB client is the ideal version for the project, and whether it's compatible with other dependencies.
  • Consider updating the check for supported architectures to be more comprehensive.
  • Check if the removal of the mysql-community-client package will cause any issues, and if so, either keep it or find a suitable replacement.

Overall, the pull request seems to be adding useful functionality to the Dockerfile. However, it's important to carefully evaluate the changes to ensure that they don't introduce any unexpected problems or conflicts.

Copy link

ti-chi-bot bot commented Nov 1, 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 description and the diff, it seems that the pull request adds the MariaDB client version 5.5.68 to the CI/CD base Docker image. The change involves downloading and installing the specified version of MariaDB client for the amd64 architecture if it is supported.

One potential problem with this pull request is that it only supports the amd64 architecture. If the Docker image is built on a different architecture, the installation step will fail. In this case, the pull request does not provide any fallback or alternative installation steps.

To fix this issue, one solution could be to check the architecture of the Docker image before downloading and installing the MariaDB client. If it is not supported, the script could exit with an appropriate error message.

Another suggestion to improve the pull request is to include a version check before installing the MariaDB client. This check will ensure that the version specified in the pull request is not already installed, preventing unnecessary installations.

Overall, the pull request seems to be a reasonable addition to the CI/CD base Docker image, but some improvements can be made to make the installation step more robust and error-proof.

@ti-chi-bot ti-chi-bot bot added the size/S label Nov 1, 2024
Copy link

ti-chi-bot bot commented Nov 1, 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, the key changes are:

  • The installation of MariaDB client version 5.5.68 is added to the Dockerfile.
  • The PATH environment variable is updated to include the new MariaDB client installation path.

One potential problem that can be identified is that the script only checks for amd64 architecture to proceed with the installation of mariadb. This means that the installation will only work for this specific architecture and may cause issues for other architectures. It would be better to add support for multiple architectures to ensure compatibility.

To fix this, the script could be updated to include checks for other architectures as well. For example, the script could check for arm64 architecture and proceed with installation accordingly. Additionally, it would be good to add tests to ensure that the installation works as expected for each architecture.

Overall, the pull request looks good, but the issue with only supporting one architecture should be addressed.

Copy link

ti-chi-bot bot commented Nov 1, 2024

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by:

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 merged commit 48abddf into PingCAP-QE:main Nov 1, 2024
12 checks passed
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