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(dockerfiles/ci/base): update ci base image #437

Merged
merged 8 commits into from
Sep 24, 2024

Conversation

wuhuizuo
Copy link
Contributor

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

Copy link

ti-chi-bot bot commented Sep 24, 2024

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

Summary:
This pull request updates the base image in the Dockerfile used for CI/CD and also updates the versions of several tools. Specifically, it updates the Go version to 1.23.1, adds support for Rust, installs MariaDB client and sets up the repository, installs Node.js, upgrades Git, and installs several Java tools including Gradle and Apache Maven. In addition, it updates the Skaffold configuration to use the new Go versions and updates the Dockerfile for CI.

Potential problems:

  • The description of the pull request is too brief and does not provide enough information about the changes made in the pull request.
  • The new Go version is not yet widely used, which may cause some compatibility issues with other dependencies.
  • The removal of old Go versions might cause some builds to break if they rely on specific version.
  • Changes to the Dockerfile may cause issues with the build process if not thoroughly tested.

Fixing suggestions:

  • The pull request description should be more detailed and provide more information about the changes made.
  • It is recommended to test the new Go version with all the dependencies to ensure compatibility before merging the pull request.
  • It is better to keep the old Go versions for a while until the majority of the dependencies are compatible with the new version.
  • The changes to the Dockerfile should be tested thoroughly before merging the pull request.

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

ti-chi-bot bot commented Sep 24, 2024

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

Summary:

This pull request updates the CI base image to a newer version. It replaces the previous Golang version with 1.23.1 and updates some other packages like MariaDB and Node.js. Also, it updates the versions of some of the tools (like bazelisk, yq, oras, codecov, gradle, apache-maven) used in the image.

Potential problems:

  • The new Golang version may cause compatibility issues with some of the dependencies. It would be better to test the code with this new version before merging the pull request.
  • The new version of Node.js may break some of the existing code if it relies on deprecated APIs.
  • The new version of Gradle may cause issues with some of the existing build scripts.

Fixing suggestions:

  • Test the code with the new Golang version to ensure compatibility.
  • Check the existing codebase for compatibility issues with the new Node.js version and update the code accordingly.
  • Update the build scripts to ensure compatibility with the new version of Gradle.

Overall, the changes look good, but it's always better to test the code and ensure compatibility before merging the pull request.

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

ti-chi-bot bot commented Sep 24, 2024

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

Summary:

  • This pull request updates the base image for the CI Dockerfile.
  • It also updates the version of Go to be used in the build process.

Potential problems:

  • The new base image may not be compatible with certain packages or tools that were previously used in the build process.
  • The updated Go version may introduce breaking changes that require code changes.

Fix suggestions:

  • Consider testing the new base image thoroughly to ensure compatibility with all necessary packages and tools.
  • Review and update any code that may be impacted by the updated Go version.
  • It may be helpful to update the pull request description with more detailed information about the changes being made.

Copy link

ti-chi-bot bot commented Sep 24, 2024

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

Summary:

This pull request updates the Dockerfile for the CI base image and adds a new Dockerfile for a Jenkins image. It also updates the Skaffold configuration to include the Jenkins image and removes the outdated Golang versions.

Potential problems:

  1. In the Skaffold configuration, the "requires" section is missing an alias for the base image, which may cause issues when trying to build the Jenkins image.
  2. The curl command in the base image's Dockerfile does not verify the SSL certificate, which may pose a security risk.
  3. The fetch-depth and fetch-tags options in the checkout step of the GitHub workflow may result in slower performance due to fetching unnecessary history and tags.

Fixing suggestions:

  1. Add an alias for the base image in the "requires" section of the Skaffold configuration.
  2. Add the --tlsv1.2 option to the curl command in the base image's Dockerfile to verify the SSL certificate.
  3. Remove the fetch-depth and fetch-tags options from the checkout step of the GitHub workflow to speed up the process.

Copy link

ti-chi-bot bot commented Sep 24, 2024

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

Summary:

This pull request updates the base image of the CI dockerfile by adding new OS packages, updating golang, nodejs and rust toolchains, and adding new tools like oras and codecov. It also adds a new dockerfile for jenkins.

Potential problems:

  1. The PR changes the golang version from 1.23.1 to 1.23.1, which might not be significant enough to justify the update.

  2. The PR adds a new dockerfile for jenkins, but it is not clear what the purpose of this dockerfile is and how it should be used.

Fixing suggestions:

  1. If there is no significant reason to update the golang version, the change should be reverted.

  2. The purpose of the jenkins dockerfile should be clarified in the PR description, and any necessary documentation or usage instructions should be added.

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

ti-chi-bot bot commented Sep 24, 2024

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

Summary:
The pull request updates the base image of the CI pipeline to Rocky Linux 8.10.20240528 and updates the versions of some of the tools such as Go, Node.js, Rust, and MariaDB. It also adds a new Dockerfile for Jenkins and modifies the Skaffold configuration to use the new base image.

Potential Problems:

  1. The fetch-depth and fetch-tags options are not used in the actions/checkout step of the pull-ci-runtime-images.yaml file. These options can help to speed up the checkout process by limiting the history depth and fetching only the necessary tags.
  2. The MariaDB-client package is installed in the base image, but there is no mention of a MariaDB server. This may cause confusion for anyone trying to use the image for database-related tasks.
  3. The skaffold.yaml file includes profiles for Go versions that have reached end-of-life and are no longer maintained. This may cause compatibility issues or security vulnerabilities.

Fixing Suggestions:

  1. Add the fetch-depth and fetch-tags options to the actions/checkout step to speed up the checkout process.
  2. Consider adding a MariaDB server to the base image or removing the MariaDB-client package if it's not necessary.
  3. Update the Skaffold profiles to use the latest supported versions of Go to avoid compatibility issues and security vulnerabilities.

Copy link

ti-chi-bot bot commented Sep 24, 2024

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

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

  • Update the base image for CI Dockerfiles, from golang:1.23.1 to quay.io/rockylinux/rockylinux:8.10.20240528.
  • Update the versions of some tools in the Dockerfiles, including go, yarn, pnpm, bazelisk, codecov, and oras.
  • Add a new Dockerfile for Jenkins, which is based on the updated CI base image and installs sudo and a jenkins user.

There are some potential problems that need to be addressed:

  • The Docker build context may have been changed by adding new files to the repository. Therefore, we need to check if there are any new files that need to be added to the build context.
  • The fetch-depth and fetch-tags options are not used consistently in the Checkout sources step of the CI workflow files. Either remove them or use them consistently in all workflow files.
  • The go-profile matrix in the CI workflow files includes versions that have reached end-of-life (EOL), such as go-1.19. It would be better to remove these EOL versions.

Here are some suggestions to fix these issues:

  • Check if there are any new files that need to be added to the build context, and add them if necessary.
  • Either remove the fetch-depth and fetch-tags options from the Checkout sources step, or use them consistently in all workflow files.
  • Remove the EOL versions from the go-profile matrix in the CI workflow files.

wuhuizuo and others added 2 commits September 24, 2024 08:42
Signed-off-by: wuhuizuo <wuhuizuo@126.com>
Copy link

ti-chi-bot bot commented Sep 24, 2024

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

This PR aims to update the Continuous Integration (CI) base image in a Dockerfile. Here are the key changes:

  1. The base module has been renamed to 'ci' in both the 'pull-ci-runtime-images.yaml' and 'release-ci-runtime-images.yaml'
  2. The Go versions used in the CI have been updated to 1.23, 1.22, and 1.21 respectively.
  3. The depth of the Git fetch has been changed to '0', and the fetch-tags option has been set to 'true'.
  4. The base Docker image has been updated from 'golang' to 'quay.io/rockylinux/rockylinux'.
  5. The Rust toolchain installation method has been changed and the installation of various other tools such as 'yq', 'oras', 'codecov', 'gradle', 'apache-maven' have been updated.
  6. A new Dockerfile has been added for Jenkins.
  7. The 'skaffold.yaml' file has been updated to reflect the changes in the Dockerfiles and the Go versions.

Potential problems:

  1. Lack of explanation: The PR description does not provide any context or explanation for the changes. It's important to explain why changes are made for the benefit of other developers and for future reference.
  2. Breaking changes: If other services or systems depend on the old versions of Go or the 'base' module, this could potentially break them.
  3. Missing test: There is no information about whether these changes were tested. For instance, if the new Docker image works as expected.

Fixing suggestions:

  1. Provide a detailed explanation of why these changes were made and how they improve the CI system in the PR description.
  2. Before merging, ensure that all systems that depend on the old Go versions or the 'base' module have been updated accordingly, or can handle these changes.
  3. Run tests to ensure the new Docker image works as expected and doesn't break anything.

Copy link

ti-chi-bot bot commented Sep 24, 2024

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

This pull request primarily updates the Continuous Integration (CI) base image of the project, making changes to various Dockerfiles, GitHub workflows, and skaffold configuration files.

Key changes include:

  1. Updating the CI base image from base to ci in both pull-ci-runtime-images.yaml and release-ci-runtime-images.yaml workflows.
  2. Updating Go versions in the testing matrix from 1.19, 1.20, 1.21 to 1.21, 1.22, 1.23.
  3. Making changes to the Dockerfile for the CI base image, such as:
    • Changing the base image from Golang to Rocky Linux.
    • Updating the installation of various tools and packages.
    • Reordering and reformatting the Dockerfile for clarity.
  4. Adding a new Dockerfile for Jenkins.
  5. Updating the skaffold.yaml configuration file.

Potential issues include:

  1. There are no unit tests or integration tests to verify these changes. To fix this, consider adding tests that verify the new images and workflows.
  2. The pull request does not specify the reason for the changes or any benefits they might bring. To fix this, consider adding a more detailed explanation in the PR description.
  3. The new base image is Rocky Linux. If the project was relying on specific behaviors of the previous base image (Golang), this might cause unforeseen issues.
  4. The PR removes the git tool upgrade step from the Dockerfile without any explanation. If this upgrade is still necessary, this could cause issues.
  5. The PR adds a new Jenkins Dockerfile but doesn't seem to use it in any workflows.

Overall, the PR needs more context and possibly some additional testing. The changes should be fine as long as they have been correctly integrated into the project and thoroughly tested.

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

ti-chi-bot bot commented Sep 24, 2024

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

Key Changes:

  1. The base image for the CI environment has been updated from golang to quay.io/rockylinux/rockylinux.
  2. The Go versions that are being targeted have been updated from 1.21, 1.20, and 1.19 to 1.23, 1.22, and 1.21.
  3. Fetch depth and tags have been added to the GitHub Actions checkout step.
  4. The Dockerfile for the CI base image has been significantly updated, with changes to installed packages, toolchains, and other configurations.
  5. A new Dockerfile for a Jenkins environment has been added.
  6. The Skaffold configuration has been updated to reflect the changes to the base image, Go versions, and the addition of the Jenkins environment.

Potential Problems:

  1. The base image change could potentially introduce unexpected behavior or incompatibilities. The new base image is a different Linux distribution that may not have the same package versions or configurations as the previous image.
  2. The updated Go versions could potentially introduce breaking changes or incompatibilities with the current codebase.
  3. The Jenkins Dockerfile is currently set up to use password-based authentication for the Jenkins user, which may not be secure enough for some environments.
  4. There's no cleanup of the package manager cache in the Dockerfiles, which could potentially result in larger image sizes.

Suggestions:

  1. Thoroughly test the new base image to ensure that it behaves as expected and does not introduce any unexpected issues.
  2. Review the Go version updates to ensure that they are compatible with the current codebase. Ensure that any necessary updates are made to maintain compatibility.
  3. Consider using SSH key-based authentication instead of password-based authentication for the Jenkins user in the Dockerfile.
  4. Add cleanup of the package manager cache in the Dockerfiles to reduce the image size. For example, if using apt, you can use apt-get clean && rm -rf /var/lib/apt/lists/* after installing packages. If using dnf, you can use dnf clean all.

Copy link

ti-chi-bot bot commented Sep 24, 2024

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

The pull request titled "feat(dockerfiles/ci/base): update ci base image" includes changes to the CI base image and related configurations.

Key changes:

  1. It updates the base image in the Dockerfile from golang to quay.io/rockylinux/rockylinux.
  2. It modifies the GitHub workflows (pull-ci-runtime-images.yaml and release-ci-runtime-images.yaml) to change the module from 'base' to 'ci' and update Go versions from 1.19-1.21 to 1.21-1.23.
  3. It adds a new Dockerfile for Jenkins under the 'ci' directory.
  4. It updates the Go versions and the names of the images in skaffold.yaml.
  5. It removes the release-build-base config from skaffold.yaml.

Potential problems:

  1. The change from Golang to Rocky Linux as the base image might have an impact on the build process or runtime environment if certain Golang-specific features were used.
  2. The new Go versions might introduce compatibility issues with the existing code.
  3. The new Jenkins Dockerfile creates a user with sudo privileges, which could pose a security risk if mishandled.
  4. Removing the release-build-base config from skaffold.yaml might affect the release build process.

Fixing suggestions:

  1. Ensure that the switch from Golang to Rocky Linux as the base image does not affect the application. Test thoroughly before merging the changes.
  2. Test the application with the new Go versions to ensure compatibility.
  3. Review the necessity of giving the Jenkins user sudo privileges. If it's necessary, ensure that proper security measures are in place.
  4. Make sure that the removal of the release-build-base config from skaffold.yaml does not affect the release build process, or provide an alternative way to handle it.

@wuhuizuo
Copy link
Contributor Author

/approve

Copy link

ti-chi-bot bot commented Sep 24, 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 Sep 24, 2024
@ti-chi-bot ti-chi-bot bot merged commit 1a06186 into main Sep 24, 2024
8 checks passed
@ti-chi-bot ti-chi-bot bot deleted the fix/update-old-base-image branch September 24, 2024 09:11
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