-
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 on Centos #1747
Supports CI on Centos #1747
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 improvements for the code patch:
-
In the
build_on_ubuntu
job, it is recommended to use theruns-on: ubuntu-latest
declaration instead ofcontainer: 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. -
There is a missing step to checkout the repository in the
build_on_centos
job. Add a step that usesactions/checkout@v2
before the "Install deps" step. This will ensure that the repository is available inside the container. -
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. -
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. -
Consider adding comments to explain the purpose of each step in the workflow to improve readability and maintainability.
-
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. -
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.
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.
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:
-
Container Image: Instead of using the
centos:7
image, it is recommended to use specific tags or versions for stability, such ascentos:7.9.2009
. This ensures consistent behavior across different executions. -
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 likepip
for Python dependencies. -
Python Dependencies: Although the code installs Python 3, it doesn't explicitly invoke the correct version of
pip
for installation. Addpython3 -m pip install redis
to ensure the dependency is installed for the right Python version. -
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. -
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. -
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.
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.
-
In the
Unit Test
step, change the command./pikatests.sh all
tosh ${{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.
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.
Code review comments:
-
Container Image:
- Changing from
image: centos:7
tocontainer: docker.io/centos:7
is a good change as it explicitly specifies the container registry location.
- Changing from
-
Dependencies Installation:
- The addition of
gcc automake make
andpython3
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.
- The addition of
-
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.
-
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.
- Make sure the shell script
-
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.
- Ensure that
-
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.
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 provided code patch, here is a brief code review:
-
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.
- It's generally a good practice to install all necessary dependencies in a single command separated by spaces, rather than multiple
-
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.
- The script
-
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.
- The script
-
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.
- It's assumed that the
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.
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's a brief code review of the provided patch:
-
The container image is changed from
centos:7
todocker.io/centos:7
. This change looks fine. -
Additional dependencies (
gcc
,automake
,make
) are installed, which seems appropriate for building the project. -
python3
andredis
are installed usingyum
, which suggests that the project might have Python and Redis-related functionality. Ensure that these dependencies are necessary for the project. -
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.
-
A line is added to print the workspace directory before building and testing. This can be helpful for debugging purposes.
-
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.
-
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.
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 seems fine, but here are a few suggestions for improvement:
-
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 ofdocker.io/centos:7
. -
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. -
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. -
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. -
The "Start pika master and slave" step could benefit from error handling and logging output for better visibility during the workflow run.
-
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.
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, here are some observations and suggestions for your code:
-
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 usecentos:7
as it is the default registry. -
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.
- Installing additional packages like
-
Working Directory: In steps where you're working with paths like
${{github.workspace}}/build
, it's best to set theworking-directory: ${{github.workspace}}/build
at the job level so that you don't need to repeat it in each step. -
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.
- You can remove the
-
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.
- Again, no need for the
-
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.
conf/pika.conf
Outdated
@@ -5,7 +5,7 @@ | |||
# Pika port, the default value is 9221. | |||
# [NOTICE] Port Magic offsets of port+1000 / port+2000 are used by Pika at present. | |||
# Port 10221 is used for Rsync, and port 11221 is used for Replication, while the listening port is 9221. | |||
port : 9221 | |||
port : 9224 | |||
|
|||
# Random value identifying the Pika server, its string length must be 40. | |||
# If not set, Pika will generate a random string with a length of 40 random characters. |
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 you provided, here's a brief review:
- Port Change: The patch suggests changing the
port
value from 9221 to 9224. This seems like a specific configuration change specific to your application or environment.
Improvement Suggestion:
- If there is a valid reason for changing the port, make sure it doesn't conflict with any other services or applications running on the system.
Overall, this patch appears to be a simple configuration adjustment. There are no bug risks apparent in the given code snippet. However, it's important to review the entire codebase and consider the context to provide a comprehensive code review.
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.
The code review of the provided code patch is as follows:
-
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.
-
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. -
In the step "Install cmake", you install CMake using the run command. This also seems appropriate.
-
The step "Build program" runs the CMake build command. It looks fine, assuming that your CMake configuration is valid.
-
In the step "Run tests," you execute the CTest command to run tests. This is generally a good practice.
-
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.
tools/pika_exporter/version.go
Outdated
BuildVersion = "Filled in by build" | ||
BuildCommitSha = "Filled in by build" | ||
BuildDate = "Filled in by build" | ||
GoVersion = "Filled in by build" | ||
) |
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.
The code patch you provided seems to contain indentation changes, but no functional changes to the code. However, there are a few indentation issues in the patch itself that may be confusing. Here's the corrected version:
package main
const (
BuildVersion = "Filled in by build"
BuildCommitSha = "Filled in by build"
BuildDate = "Filled in by build"
GoVersion = "Filled in by build"
)
In terms of code review and potential improvements, since this code snippet only defines constants, there isn't much to evaluate in terms of correctness or bugs. However, here are some general suggestions:
- Ensure that the values assigned to the constants are being properly filled in during the build process.
- Consider providing more descriptive names for the constants if possible.
- If any of these constants are expected to have different values based on the build environment (e.g., production vs. development), consider using build tags or flags to handle those cases.
Since this code snippet is limited in scope, it's difficult to provide further feedback without additional context or code.
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 your code patch:
-
In the
Install deps
step, you have added the installation ofgcc
,automake
,make
, andtar
. It's good to include these dependencies if they are required for your project. -
You have added the installation of
python3
and the Redis library usingpip
. 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. -
In the
Install cmake
step, there doesn't seem to be any additional changes or improvements needed. -
In the
Build and Test
step, you can remove thecd ${{github.workspace}}/build
command since you're already in the correct directory in the previous step. -
In the
Unit Test
step, you can remove thecd ${{github.workspace}}
line since you're already in that directory when the step starts. Additionally, uncommenting the linesh ${{github.workspace}}/pikatests.sh all
would execute the 'pikatests.sh' script, so make sure it's intended. -
In the
Start pika master and slave
step, you don't need tocd ${{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. -
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.
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 with a few potential improvements and bug risks to consider:
-
In the "Install deps" step, you install additional dependencies like
gcc
andautomake
, but it's not clear if they are needed by your project. Make sure these dependencies are necessary for your build process. -
The step "Install cmake" seems unnecessary since it's already available in the container image. It can be removed.
-
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 thecmake
command. -
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. -
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. -
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. -
Consider adding error handling and logging throughout the workflow to provide better visibility into any potential issues during the execution of each step.
-
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.
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 seems fine with a few improvement suggestions:
-
Add comments: It's helpful to add comments throughout the workflow to explain what each step is doing or any specific considerations.
-
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.
-
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. -
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.
-
Ensure Python environment consistency: When installing dependencies, ensure that the Python environment is consistent. For example, you install
python3-pip
usingyum
but later usepython3 -m pip
to upgrade and install packages. It's recommended to have a consistent approach. -
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.
-
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. -
Improve naming conventions: The step names could be more descriptive, such as "Cache dependencies" instead of just "cache dependencies."
-
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.
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 and potential improvements for your code:
-
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. -
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. -
Combine multiple
yum install
commands into a single command by separating them with spaces. This can make the process more efficient. -
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. -
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. -
It seems like there are some redundant lines related to enabling devtoolset-10. Make sure these commands are necessary and eliminate any duplicates.
-
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.
-
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.
Supports CI on Centos
fix #1692