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 on Centos #1747

Closed
wants to merge 3 commits into from
Closed

Supports CI on Centos #1747

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
824666c
supports CI on centos
Mixficsol Jul 17, 2023
ce008b8
Supports CI on Centos
Mixficsol Jul 18, 2023
f9854e0
code format3
Mixficsol Jul 18, 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
30 changes: 26 additions & 4 deletions .github/workflows/pika.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,22 +76,23 @@ jobs:
# 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
# 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 git autoconf centos-release-scl
yum install -y wget git autoconf centos-release-scl gcc automake make
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
run: |
Expand Down Expand Up @@ -129,3 +130,24 @@ jobs:
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 improvements for the code patch:

  1. In the build_on_ubuntu job, it is recommended to use the runs-on: ubuntu-latest declaration instead of container: docker.io/centos:7, as the latter specifies a CentOS container while the job name suggests an Ubuntu environment. Confirm that you want to use Ubuntu or adjust the configuration accordingly.

  2. There is a missing step to checkout the repository in the build_on_centos job. Add a step that uses actions/checkout@v2 before the "Install deps" step. This will ensure that the repository is available inside the container.

  3. In the "Install deps" step of the build_on_centos job, additional dependencies are being installed. Make sure these dependencies are necessary for your project. If they are not required, you can remove those lines from the script.

  4. The installation of Python 3 and Redis in the build_on_centos job seems unrelated to the build process. Verify if these installations are necessary or if they can be moved to a different job or step.

  5. Consider adding comments to explain the purpose of each step in the workflow to improve readability and maintainability.

  6. In the "Unit Test" step, make sure that the script pikatests.sh exists and is expected to be in the root directory of the project. Otherwise, adjust the path accordingly.

  7. The "Start pika master and slave" and "Run Python E2E Tests" steps assume specific locations for the scripts/files being executed. Verify that the paths are correct based on your project's structure.

Overall, it's important to thoroughly test the workflow after implementing these changes to ensure it runs smoothly and meets your requirements.

Copy link

Choose a reason for hiding this comment

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

The code patch appears to be a GitHub Actions workflow that builds, tests, and runs integration tests for a project. Here are some suggestions for improvement and potential bug risks:

  1. Container Image: Instead of using the centos:7 image, it is recommended to use specific tags or versions for stability, such as centos:7.9.2009. This ensures consistent behavior across different executions.

  2. Dependency Installation: While installing dependencies with yum commands is generally fine, it can be risky if package versions or availability change. Consider specifying explicit versions or using a package manager like pip for Python dependencies.

  3. Python Dependencies: Although the code installs Python 3, it doesn't explicitly invoke the correct version of pip for installation. Add python3 -m pip install redis to ensure the dependency is installed for the right Python version.

  4. Unit Test Step: Ensure that the pikatests.sh script is present in the specified path (./pikatests.sh all). Also, verify the file permissions and execution requirements before running the script.

  5. Start Pika Master and Slave: Make sure that the start_master_and_slave.sh script exists in the specified path (../tests/integration/start_master_and_slave.sh). Confirm the script's permissions and whether any environment variables need to be set before executing.

  6. E2E Tests: Verify that the pika_replication_test.py test file is present in the specified path (../tests/integration/pika_replication_test.py). Additionally, consider setting up the necessary prerequisites, such as configuring databases properly, before running these tests.

Remember to thoroughly test the workflow, ensuring that all required scripts and files exist in their expected locations, and monitor for any error messages during execution.

Copy link

Choose a reason for hiding this comment

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

  • In the Unit Test step, change the command ./pikatests.sh all to sh ${{github.workspace}}/pikatests.sh all. This ensures that the script is executed using the shell specified in the shebang line (e.g., #!/bin/sh) of the script.

  • In the Start pika master and slave step, change the command ../tests/integration/start_master_and_slave.sh to ./../tests/integration/start_master_and_slave.sh. This ensures that the script is executed with the correct path relative to the current working directory.

  • In the Run Python E2E Tests step, there are no specific improvements or bug risks observed.

Overall, the additional changes include adding cd ${{github.workspace}} at the beginning of each step to ensure the correct working directory is set before executing commands.

Copy link

Choose a reason for hiding this comment

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

Code review comments:

  1. Container Image:

    • Changing from image: centos:7 to container: docker.io/centos:7 is a good change as it explicitly specifies the container registry location.

  2. Dependencies Installation:

    • The addition of gcc automake make and python3 installations is good for building and running the code.
    • Consider adding specific versions or package managers (e.g., yum install -y python3-pip) for better control over dependency versions.

  3. Unit Test and Integration Tests:

    • It's helpful to have separate steps for running unit tests and integration tests.
    • Ensure that the shell script pikatests.sh is present in the expected location ($github.workspace) and has appropriate permissions.

  4. Start Pika Master and Slave:

    • Make sure the shell script start_master_and_slave.sh is present in the correct location.
    • Verify that the script is designed to work within the GitHub Actions environment.

  5. Run Python E2E Tests:

    • Ensure that pika_replication_test.py is available in the expected location.
    • Check if the Python dependencies required by pika_replication_test.py are installed within the container.

  6. Overall Workflow Structure:

    • The workflow looks well-structured with clear steps.
    • Consider using specific version tags for external tools or libraries to ensure consistent behavior and avoid potential compatibility issues.

Bug risk and improvements:

  • Verify that all paths and file locations referenced in the workflow correspond correctly with the project structure and repository content.
  • Consider adding error handling and logging statements to provide better visibility into the workflow execution.
  • Implement proper error handling for each step, including notifying the relevant parties about failed jobs or tests.
  • Ensure that the necessary configuration files and setup instructions are provided alongside the codebase.

These suggestions should help you improve the code review. Remember to adapt them to your specific project's requirements and best practices.

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 provided code patch, here is a brief code review:

  1. In the "Install deps" step:

    • It's generally a good practice to install all necessary dependencies in a single command separated by spaces, rather than multiple yum install commands.
    • It seems that Python and Redis are required dependencies. Assuming they are needed for the subsequent steps, it's better to install them here.

  2. In the "Unit Test" step:

    • The script pikatests.sh is being executed without any validation or error handling. Make sure the script is expected to run successfully.
    • Consider adding an explicit check/verification/assertion after running the script to ensure the expected results are obtained.

  3. In the "Start pika master and slave" step:

    • The script start_master_and_slave.sh is being executed without any validation or error handling. Make sure the script is expected to run successfully.
    • Consider adding an explicit check to ensure the Pika master and slave servers are started before proceeding to the next step.

  4. In the "Run Python E2E Tests" step:

    • It's assumed that the pika_replication_test.py script is expected to run successfully. Ensure it exists and executes as intended.

Other than those points, the code patch appears to be functional. It sets up the necessary dependencies and executes different steps for building, testing, and running the application. However, without further context about the project or specific requirements, it's challenging to provide more detailed improvement suggestions.

Copy link

Choose a reason for hiding this comment

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

Here's a brief code review of the provided patch:

  1. The container image is changed from centos:7 to docker.io/centos:7. This change looks fine.

  2. Additional dependencies (gcc, automake, make) are installed, which seems appropriate for building the project.

  3. python3 and redis are installed using yum, which suggests that the project might have Python and Redis-related functionality. Ensure that these dependencies are necessary for the project.

  4. The installation steps appear to be sequential and logically correct, but it would be good to add error handling or exit codes in case any of the steps fail.

  5. A line is added to print the workspace directory before building and testing. This can be helpful for debugging purposes.

  6. Two new steps are introduced, "Unit Test" and "Start pika master and slave," but they are commented out. Uncomment them if they should be included as part of your workflow.

  7. Finally, a step is added to run Python E2E (end-to-end) tests. Make sure the path to the test script (../tests/integration/pika_replication_test.py) is correct and aligned with your project structure.

Improvement Suggestions:

  • Consider adding error handling and appropriate exit codes for each step.
  • Add comments or explanation for configuration settings to make the code more understandable.
  • Consider using separate job stages for building, testing, and deploying (if applicable) to organize the workflow better.
  • If there are other defined scripts (pikatests.sh, start_master_and_slave.sh), ensure that their paths and usage are correct.

Please note that a comprehensive code review requires a deeper understanding of the project and its requirements.

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 seems fine, but here are a few suggestions for improvement:

  1. Consider using a specific tag for the CentOS container image instead of relying on the "latest" tag. This ensures reproducibility and avoids potential compatibility issues if the latest image changes. For example, use docker.io/centos:7.9.2009 instead of docker.io/centos:7.

  2. Add error handling in case any of the package installations fail. You can use set -e at the beginning of the script to stop execution if any command returns a non-zero exit code.

  3. Instead of sourcing Devtoolset-10 environment variables in each step, you can define them once globally in the workflow or use an environment file (.env) loaded at the beginning of the workflow.

  4. In the "Unit Test" step, it's unclear why the pikatests.sh script is not executed as commented out with #. If there's a specific reason, consider providing a comment explaining it.

  5. The "Start pika master and slave" step could benefit from error handling and logging output for better visibility during the workflow run.

  6. The "Run Python E2E Tests" step seems fine, but ensure that the required dependencies and prerequisites are clearly documented in your project's README or corresponding documentation.

Remember to thoroughly test the workflow after making any changes to ensure everything works correctly.

Copy link

Choose a reason for hiding this comment

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

Overall, here are some observations and suggestions for your code:

  1. Container Image: It's a good practice to specify the full image name, including the registry. Instead of using docker.io/centos:7, you can simply use centos:7 as it is the default registry.

  2. Step: Install deps:

    • Installing additional packages like gcc, automake, make, tar is a good improvement suggestion.
    • Installing Python 3 and Redis with python3 -m pip install redis can be moved to a separate step for better organization.

  3. Working Directory: In steps where you're working with paths like ${{github.workspace}}/build, it's best to set the working-directory: ${{github.workspace}}/build at the job level so that you don't need to repeat it in each step.

  4. Unit Test Step:

    • You can remove the echo statements used for debugging unless they serve a specific purpose.
    • Uncommenting the line sh ${{github.workspace}}/pikatests.sh all would make more sense, assuming it is necessary for running the desired tests.

  5. Start pika master and slave Step:

    • Again, no need for the echo statements used for debugging.
    • Make sure the path to start_master_and_slave.sh script is correct and accessible.

  6. Run Python E2E Tests Step: Similar to the previous steps, remove unnecessary echo statements.

Reviewing the provided code patch, these would be the main improvements and suggestions. Of course, it heavily depends on the specific requirements and functionalities you're aiming to achieve with this code.

Copy link

Choose a reason for hiding this comment

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

The code review of the provided code patch is as follows:

  1. In the step "Install deps", you have added the installation commands for CentOS dependencies. It seems appropriate, but ensure that these are the correct dependencies required for your project.

  2. In the same step, you install Python 3 and then use it to install the Redis package (python3 -m pip install redis). This looks fine if you need Redis in your project. Ensure that Redis is a necessary dependency for your project.

  3. In the step "Install cmake", you install CMake using the run command. This also seems appropriate.

  4. The step "Build program" runs the CMake build command. It looks fine, assuming that your CMake configuration is valid.

  5. In the step "Run tests," you execute the CTest command to run tests. This is generally a good practice.

  6. In the new steps "Unit Test," "Start pika master and slave," and "Run Python E2E Tests," you perform additional actions related to unit testing, starting Pika (a Redis protocol-compatible server), and running Python end-to-end tests. Please make sure that these steps are relevant for your project and that the paths and scripts you're using are correct.

Overall, the code patch appears to be well-structured and follows the standard approach of installing dependencies, building the project, and running tests. However, it's essential to ensure that the specific dependencies, paths, and scripts being used are valid and suitable for your 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 observations and suggestions for your code patch:

  1. In the Install deps step, you have added the installation of gcc, automake, make, and tar. It's good to include these dependencies if they are required for your project.

  2. You have added the installation of python3 and the Redis library using pip. This is fine if your project requires Python and Redis. Make sure to specify the version of Redis library you want to install, or remove it if it's not necessary.

  3. In the Install cmake step, there doesn't seem to be any additional changes or improvements needed.

  4. In the Build and Test step, you can remove the cd ${{github.workspace}}/build command since you're already in the correct directory in the previous step.

  5. In the Unit Test step, you can remove the cd ${{github.workspace}} line since you're already in that directory when the step starts. Additionally, uncommenting the line sh ${{github.workspace}}/pikatests.sh all would execute the 'pikatests.sh' script, so make sure it's intended.

  6. In the Start pika master and slave step, you don't need to cd ${{github.workspace}}/build again since you're already in that directory. Also, ensure that the path to the 'start_master_and_slave.sh' script is correct.

  7. In the Run Python E2E Tests step, similar to the previous step, you don't need to change the directory again. Verify that the path to the 'pika_replication_test.py' script is accurate.

Overall, the code patch seems fine with the given information. Ensure that the paths and dependencies are correctly configured, and thoroughly test the workflow to catch any potential bugs or issues.

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 with a few potential improvements and bug risks to consider:

  1. In the "Install deps" step, you install additional dependencies like gcc and automake, but it's not clear if they are needed by your project. Make sure these dependencies are necessary for your build process.

  2. The step "Install cmake" seems unnecessary since it's already available in the container image. It can be removed.

  3. In the "Build your program" step, it's a good practice to use mkdir -p build && cd build to create the build directory if it doesn't exist before running the cmake command.

  4. In the "Unit Test" step, the command chmod +x ${{github.workspace}}/pikatests.sh may not work as expected if the shell script is not located at that path. Double-check the path and ensure it's correct.

  5. For the "Start pika master and slave" step, make sure that the ../tests/integration/start_master_and_slave.sh script exists and has the necessary permissions for execution. Verify the path and permissions beforehand.

  6. Similarly, in the "Run Python E2E Tests" step, confirm that the ../tests/integration/pika_replication_test.py script is present and accessible. Ensure that the Python environment has the required dependencies installed to run the script successfully.

  7. Consider adding error handling and logging throughout the workflow to provide better visibility into any potential issues during the execution of each step.

  8. It's good practice to include comments explaining each step's purpose to enhance code readability and facilitate future maintenance.

Remember to thoroughly test the workflow after making any changes to ensure it functions as expected.

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 seems fine with a few improvement suggestions:

  1. Add comments: It's helpful to add comments throughout the workflow to explain what each step is doing or any specific considerations.

  2. Separate tasks into smaller steps: Instead of having a single step for "Install deps," consider breaking it down into separate steps for better readability and error handling.

  3. Use an action for dependency caching: Consider using the actions/cache action to cache dependencies instead of manually installing them every time. This will speed up subsequent workflow runs.

  4. Utilize matrix builds: If cross-platform coverage is needed, you can convert the build_on_centos job into a matrix build using different platforms (Windows, Mac). This will allow testing on multiple operating systems.

  5. Ensure Python environment consistency: When installing dependencies, ensure that the Python environment is consistent. For example, you install python3-pip using yum but later use python3 -m pip to upgrade and install packages. It's recommended to have a consistent approach.

  6. Comment or remove unused code: There seems to be commented-out code related to cache keys in the "cache dependencies" step. Ensure to clean up unused code to improve clarity.

  7. Consider linting and static analysis: You could consider adding linting and static analysis to the CI pipeline to catch potential code issues early. Tools like clang-tidy could be utilized.

  8. Improve naming conventions: The step names could be more descriptive, such as "Cache dependencies" instead of just "cache dependencies."

  9. Handle step failures gracefully: If any step fails, it's important to handle those failures gracefully. You can use error handling mechanisms provided by GitHub Actions, such as catch errors or fail fast, to stop the execution if a critical step fails.

Remember, these are general suggestions, and the applicability may depend on specific requirements and constraints.

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 and potential improvements for your code:

  1. Add a uses: actions/checkout@v3 step at the beginning of each job to ensure that the code is checked out before running any other steps.

  2. Utilize the actions/cache@v2 action to cache dependencies. This can speed up subsequent runs by avoiding reinstallation of dependencies if nothing has changed. Make sure to specify a unique key for the cache, such as a combination of the dependencies and the operating system being used.

  3. Combine multiple yum install commands into a single command by separating them with spaces. This can make the process more efficient.

  4. Consider using a package manager (such as pip) to install Python dependencies instead of using yum. It's generally recommended to use consistent package management within a project.

  5. Instead of hard-coding the specific version of Clang (llvm-toolset-7), consider using variables or parameters to make it easier to update in the future.

  6. It seems like there are some redundant lines related to enabling devtoolset-10. Make sure these commands are necessary and eliminate any duplicates.

  7. Look into the possibility of creating separate jobs or steps for unit testing, starting pika master and slave, and running Python E2E tests. This can make the workflow more modular and easier to maintain.

  8. Consider adding error handling and notifications, so you will be alerted if any step fails.

Remember to test the workflow thoroughly and iterate on it based on feedback and requirements specific to your project.

Loading