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.

Sign up for GitHub

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

Jump to bottom

Supports CI running on Centos #1745

Closed
wants to merge 4 commits into from
Closed

Supports CI running on Centos #1745

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
48bc4b5
Supports compilation on Centos
Mixficsol Jul 16, 2023
1a070a3
Add test
Mixficsol Jul 16, 2023
951a952
Merge branch 'unstable' of github.com:OpenAtomFoundation/pika into ce…
Mixficsol Jul 16, 2023
ac034af
Merge branch 'unstable' of github.com:OpenAtomFoundation/pika into ce…
Mixficsol Jul 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 30 additions & 6 deletions .github/workflows/pika.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ env:
BUILD_TYPE: RelWithDebInfo

jobs:
build:
# The CMake configure and build commands are platform agnostic and should work equally well on Windows or Mac.
build_on_ubuntu:
# The CMake configure and build commands are platform-agnostic and should work equally well on Windows or Mac.
# You can convert this to a matrix build if you need cross-platform coverage.
# See: https://docs.github.com/en/free-pro-team@latest/actions/learn-github-actions/managing-complex-workflows#using-a-build-matrix
runs-on: ubuntu-latest
Expand Down Expand Up @@ -72,25 +72,28 @@ jobs:


build_on_centos:
# The CMake configure and build commands are platform agnostic and should work equally well on Windows or Mac.
# The CMake configure and build commands are platform-agnostic and should work equally well on Windows or Mac.
# You can convert this to a matrix build if you need cross-platform coverage.
# See: https://docs.github.com/en/free-pro-team@latest/actions/learn-github-actions/managing-complex-workflows#using-a-build-matrix
runs-on: ubuntu-latest
container:
image: centos:7
container: docker.io/centos:7

steps:
- name: Install deps
uses: actions/checkout@v2
# Configure CMake in a 'build' subdirectory. `CMAKE_BUILD_TYPE` is only required if you are using a single-configuration generator such as make.
# See https://cmake.org/cmake/help/latest/variable/CMAKE_BUILD_TYPE.html?highlight=cmake_build_type
run: |
yum install -y wget tar gcc automake autoconf libtool make
yum install -y wget git autoconf centos-release-scl
yum install -y devtoolset-10-gcc devtoolset-10-gcc-c++ devtoolset-10-make devtoolset-10-bin-util
yum install -y llvm-toolset-7
yum install -y llvm-toolset-7-clang
source /opt/rh/devtoolset-10/enable
gcc --version
make --version
yum install -y python3
python3 --version
python3 -m pip install redis


- name: Install cmake
Expand Down Expand Up @@ -128,4 +131,25 @@ jobs:
cd ${{github.workspace}}/build
source /opt/rh/devtoolset-10/enable
ctest -C ${{env.BUILD_TYPE}}

- name: Unit Test
working-directory: ${{github.workspace}}
run: |
cd ${{github.workspace}}
chmod +x ./pikatests.sh all
./pikatests.sh all

# master on port 9221, slave on port 9231, all with 2 db
- name: Start pika master and slave
working-directory: ${{github.workspace}}/build
run: |
cd ${{github.workspace}}/build
chmod +x ../tests/integration/start_master_and_slave.sh
../tests/integration/start_master_and_slave.sh

- name: Run Python E2E Tests
working-directory: ${{github.workspace}}/build
run: |
cd ${{github.workspace}}/build
python3 ../tests/integration/pika_replication_test.py

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are some observations and suggestions for the code patch:

  1. In the first job (build_on_ubuntu), the line runs-on: ubuntu-latest can be kept since it indicates that the job will run on an Ubuntu environment.

  2. It's a good practice to echo the value of ${{github.workspace}} before running commands in the Unit Test step. This can help with troubleshooting and ensure the correct directory is being used.

  3. Consider adding more descriptive names to the jobs to improve readability and understanding of their purpose.

  4. In the build_on_centos job, the line container:image:centos:7 has been removed, but there is no alternative provided. If there is a specific reason for this change, make sure to address it or find an appropriate substitution if necessary.

  5. In the Install deps step, consider specifying the version of yum install command to ensure reproducibility and compatibility with the required dependencies.

  6. The Install cmake step could benefit from specifying a version as well to ensure consistency.

  7. It's a good practice to include cleanup steps after the build process, such as removing unnecessary files, cleaning up temporary resources, or deleting any artifacts that are not needed.

  8. In the Run CTest step, consider including the BUILD_TYPE environment variable when running ctest to ensure consistency with the build configuration.

  9. For the Unit Test, Start pika master and slave, and Run Python E2E Tests steps, consider using the full path to the scripts or ensuring that the working directory has already been set correctly.

  10. Ensure that any sensitive information or credentials are properly protected and not exposed within the code patch or workflow file.

  11. Consider incorporating automated testing frameworks such as static analysis tools, unit tests, and linting tools into the workflow.

Keep in mind these suggestions may vary based on the specific requirements and context of the project.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are some suggestions for the code patch:

  1. The job names build_on_ubuntu and build_on_centos could be more descriptive to reflect their purpose. Consider naming them based on the specific build or platform they target.

  2. In the build_on_ubuntu job, you're referencing ubuntu-latest as the runs-on platform, but the job itself seems to be intended for running CentOS containers, as indicated by the Docker command to run a CentOS container. Consider updating the runs-on platform to a CentOS image if that is the intended behavior.

  3. In the build_on_centos job, you're using ubuntu-latest as the runs-on platform. If the intention is to run CentOS commands, you should update the runs-on platform to a CentOS image.

  4. It's a good practice to check out the code before starting any steps in the workflow. Add a step to check out the code using the actions/checkout@v2 action in both build_on_ubuntu and build_on_centos jobs.

  5. In the build_on_centos job, there's a step to run a CentOS container using Docker. Make sure to update the container image tag to match the desired version of CentOS (e.g., docker.io/centos:7 for CentOS 7).

  6. Consider splitting the long run commands into multiple lines for improved readability. You can use the pipe (|) symbol at the end of each line to indicate a multiline string.

  7. Ensure that the path specified for mounting the Pika project files into the CentOS container is correct and matches the actual path on your system.

  8. Make sure to clean up the CentOS container after the tests or provide appropriate cleanup steps.

  9. In the Unit Test and Start pika master and slave steps, it's good to provide more details about what these steps do. Consider adding comments or brief descriptions for clarity.

  10. Double-check the paths and file permissions for the shell scripts (pikatests.sh, start_master_and_slave.sh), and ensure they exist in the specified locations.

  11. In the Run Python E2E Tests step, make sure the path to pika_replication_test.py is correct.

  12. Overall, it appears that the provided code patch may have some inconsistencies and incomplete details. It's recommended to review and update the code based on your specific requirements and the desired workflow logic.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, the code patch looks fine, but here are a few suggestions for improvement and potential bug risks:

  1. In the build_on_ubuntu job, there is a comment suggesting converting it to a matrix build for cross-platform coverage. It would be helpful to add an explanation or a link to further documentation on how to achieve that.

  2. In the build_on_centos job, the container's image is specified as docker.io/centos:7, which looks correct. However, the actions/checkout@v2 step is missing, which indicates that the repository code is not being fetched or checked out before building. Make sure to include this step before trying to configure and build with CMake.

  3. In the same build_on_centos job, there are multiple yum install commands to install various dependencies like wget, tar, etc. Although these commands can work, it would be more appropriate to create a separate Dockerfile and use that to build a custom CentOS-based container image with all the necessary dependencies preinstalled. This way, you have better control over the environment and avoid any inconsistencies caused by relying on external repositories.

  4. After installing Python 3 using yum install -y python3, it's a good practice to ensure the installed version matches the desired version. Adding python3 --version is useful for verifying that Python 3 has been successfully installed.

  5. In the Unit Test step, it executes chmod +x ./pikatests.sh all followed by ./pikatests.sh all. Make sure that pikatests.sh script exists in the correct location and is correctly executable. Additionally, double-check whether specifying all as an argument is the intended behavior for running the tests.

  6. In the Start pika master and slave step, it executes chmod +x ../tests/integration/start_master_and_slave.sh followed by ../tests/integration/start_master_and_slave.sh. Similar to the previous point, make sure the script exists and is executable in the expected location.

  7. In the Run Python E2E Tests step, it executes python3 ../tests/integration/pika_replication_test.py. Ensure the specified Python test file is present in the correct location and that it covers the desired end-to-end testing scenario.

Remember to thoroughly test these workflow steps and verify that dependencies, scripts, and test files are correctly set up to achieve the desired outcomes.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the code patch, here are some observations and suggestions:

  1. In the build_on_ubuntu job, the comment mentions converting to a matrix build for cross-platform coverage but it still only runs on ubuntu-latest. If you want cross-platform coverage, you should consider adding jobs that run on different operating systems (e.g., Windows and macOS) using appropriate runners.

  2. In the build_on_centos job, instead of using ubuntu-latest as the runner, you should use centos-latest or specify a specific version of CentOS. Also, the container field is not needed since the container keyword is not supported in GitHub Actions. Instead, you can use a relevant base Docker image like docker.io/centos:7.

  3. The steps in the build_on_centos job include installing various dependencies like wget, tar, gcc, etc. It's a good practice to mention these dependencies in the project documentation or requirements file, so other developers know what is required for building and running the project.

  4. The step for installing Python 3 and Redis appears to be more related to the project itself rather than the code review. However, make sure you have documented the requirement for Python 3 and Redis in your project's README or requirements file.

  5. The step for running unit tests (Unit Test) seems fine, but ensure that the pikatests.sh script is available and properly tested.

  6. Starting Pika master and slave in the Start pika master and slave step seems project-specific and not directly related to the code review. However, ensure that the script start_master_and_slave.sh is available and properly tested.

  7. Running Python E2E tests in the Run Python E2E Tests step also seems project-specific. Make sure the pika_replication_test.py script exists and has been thoroughly tested.

Overall, the code patch seems to include necessary build steps, dependency installations, and test executions. Ensure that the referenced scripts exist, are accessible, and have been thoroughly tested to reduce the risk of bugs. Labeling or organizing the code review sections would further improve readability and clarity of the review process.