-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Supports CI running on Centos #1745
Conversation
run: | | ||
cd ${{github.workspace}}/build | ||
python3 ../tests/integration/pika_replication_test.py | ||
|
There was a problem hiding this comment.
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:
-
In the first job (
build_on_ubuntu
), the lineruns-on: ubuntu-latest
can be kept since it indicates that the job will run on an Ubuntu environment. -
It's a good practice to echo the value of
${{github.workspace}}
before running commands in theUnit Test
step. This can help with troubleshooting and ensure the correct directory is being used. -
Consider adding more descriptive names to the jobs to improve readability and understanding of their purpose.
-
In the
build_on_centos
job, the linecontainer: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. -
In the
Install deps
step, consider specifying the version ofyum install
command to ensure reproducibility and compatibility with the required dependencies. -
The
Install cmake
step could benefit from specifying a version as well to ensure consistency. -
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.
-
In the
Run CTest
step, consider including theBUILD_TYPE
environment variable when runningctest
to ensure consistency with the build configuration. -
For the
Unit Test
,Start pika master and slave
, andRun Python E2E Tests
steps, consider using the full path to the scripts or ensuring that the working directory has already been set correctly. -
Ensure that any sensitive information or credentials are properly protected and not exposed within the code patch or workflow file.
-
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.
run: | | ||
cd ${{github.workspace}}/build | ||
python3 ../tests/integration/pika_replication_test.py | ||
|
There was a problem hiding this comment.
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:
-
The job names
build_on_ubuntu
andbuild_on_centos
could be more descriptive to reflect their purpose. Consider naming them based on the specific build or platform they target. -
In the
build_on_ubuntu
job, you're referencingubuntu-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. -
In the
build_on_centos
job, you're usingubuntu-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. -
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 bothbuild_on_ubuntu
andbuild_on_centos
jobs. -
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). -
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. -
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.
-
Make sure to clean up the CentOS container after the tests or provide appropriate cleanup steps.
-
In the
Unit Test
andStart 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. -
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. -
In the
Run Python E2E Tests
step, make sure the path topika_replication_test.py
is correct. -
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.
run: | | ||
cd ${{github.workspace}}/build | ||
python3 ../tests/integration/pika_replication_test.py | ||
|
There was a problem hiding this comment.
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:
-
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. -
In the
build_on_centos
job, the container's image is specified asdocker.io/centos:7
, which looks correct. However, theactions/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. -
In the same
build_on_centos
job, there are multipleyum install
commands to install various dependencies likewget
,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. -
After installing Python 3 using
yum install -y python3
, it's a good practice to ensure the installed version matches the desired version. Addingpython3 --version
is useful for verifying that Python 3 has been successfully installed. -
In the
Unit Test
step, it executeschmod +x ./pikatests.sh all
followed by./pikatests.sh all
. Make sure thatpikatests.sh
script exists in the correct location and is correctly executable. Additionally, double-check whether specifyingall
as an argument is the intended behavior for running the tests. -
In the
Start pika master and slave
step, it executeschmod +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. -
In the
Run Python E2E Tests
step, it executespython3 ../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.
run: | | ||
cd ${{github.workspace}}/build | ||
python3 ../tests/integration/pika_replication_test.py | ||
|
There was a problem hiding this comment.
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:
-
In the
build_on_ubuntu
job, the comment mentions converting to a matrix build for cross-platform coverage but it still only runs onubuntu-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. -
In the
build_on_centos
job, instead of usingubuntu-latest
as the runner, you should usecentos-latest
or specify a specific version of CentOS. Also, thecontainer
field is not needed since thecontainer
keyword is not supported in GitHub Actions. Instead, you can use a relevant base Docker image likedocker.io/centos:7
. -
The steps in the
build_on_centos
job include installing various dependencies likewget
,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. -
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.
-
The step for running unit tests (
Unit Test
) seems fine, but ensure that thepikatests.sh
script is available and properly tested. -
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 scriptstart_master_and_slave.sh
is available and properly tested. -
Running Python E2E tests in the
Run Python E2E Tests
step also seems project-specific. Make sure thepika_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.
fix #1692