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: Supports Centos on CI #1772

Merged
merged 7 commits into from
Jul 21, 2023
Merged

feat: Supports Centos on CI #1772

merged 7 commits into from
Jul 21, 2023

Conversation

Mixficsol
Copy link
Collaborator

@Mixficsol Mixficsol commented Jul 20, 2023

Supports Centos on CI
feat: #1692
feat: #1779

ls -la # Optional: List files in the directory for verification
chmod +x pikatests.sh
sh pikatests.sh all

build_on_macos:
runs-on: macos-latest

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 good. Here are a few suggestions and improvements:

  1. Add comments: Although there are some comments explaining the purpose of certain actions, it would be helpful to add more comments throughout the workflow to enhance readability and make it easier for others to understand.

  2. Use specific job names: Instead of using generic names like build_on_ubuntu and build_on_macos, consider using specific names that reflect the purpose of each job. This will make it easier to identify and understand the different jobs in the workflow.

  3. Consider matrix builds: The comment suggests converting the build into a matrix build if cross-platform coverage is needed. This could be a good improvement, especially if you want to test the code on multiple platforms efficiently.

  4. Check if zip program is available: Before running the zip command, you might want to check if the zip program is available in the environment. You can use the which command to verify its existence before proceeding with zipping.

  5. Error handling: Ensure proper error handling in case any of the steps fail by using set -e at the beginning of your shell script. This will cause the script to exit immediately if any command fails, preventing further execution of potentially erroneous commands.

  6. Validation for the artifact download: After unzipping the workspace artifact, consider adding validation steps such as verifying specific files or running additional checks to ensure the artifact was downloaded and extracted correctly.

  7. Add artifact retention policy: Consider adding an artifact retention policy to specify how long the artifacts should be retained. This will prevent consuming excessive storage space over time.

Remember to test the updated workflow thoroughly to ensure all the modifications work as expected.

@Mixficsol Mixficsol changed the title support centos on ci feat: Support centos on CI Jul 20, 2023
@Mixficsol Mixficsol changed the title feat: Support centos on CI feat: Supports Centos on CI Jul 20, 2023
@@ -145,7 +187,6 @@ jobs:
key: ${{ runner.os }}-dependencies

- name: install Deps
if: ${{ steps.cache.outputs.cache-hit != 'true' }}
run: |
brew update
brew install autoconf protobuf python llvm wget git
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:

  1. Renaming "build" job: The job named "build" has been renamed to "build_on_ubuntu," which seems reasonable.

  2. Zipping and uploading workspace directory: The patch adds steps to zip the entire workspace directory and upload it as an artifact. This can be helpful for archiving and sharing the workspace.

  3. Installing additional dependencies: The patch adds which, tcl, and unzip packages installation on the CentOS-based system, which seems appropriate for compatibility and extracting the uploaded artifacts.

  4. Downloading and unpacking artifacts: The patch introduces steps to create an artifact directory, download the workspace artifact from a previous job, unzip it, and perform unit tests on the extracted files. This allows accessing and using the uploaded artifact in subsequent steps of the workflow.

Overall, the code patch appears to address archiving workspace, adding necessary dependencies, and handling artifacts effectively. However, without reviewing the entire workflow file, it's challenging to provide a comprehensive assessment. Make sure to validate and test the workflow thoroughly with different scenarios to ensure its correctness and functionality.

@@ -145,7 +192,6 @@ jobs:
key: ${{ runner.os }}-dependencies

- name: install Deps
if: ${{ steps.cache.outputs.cache-hit != 'true' }}
run: |
brew update
brew install autoconf protobuf python llvm wget git
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 code review suggestions and improvements:

  1. Job name consistency: Consider renaming the job build_on_ubuntu to maintain consistency with build_on_macos.

  2. Artifact organization: Instead of creating a new subdirectory for artifacts in the job (/github/workspace/artifacts), you can directly download and extract the workspace artifact to the workspace directory (${{ github.workspace }}). This simplifies the overall artifact management.

  3. Removing unnecessary steps:

    • The step that checks for cache-hit (if: ${{ steps.cache.output.cache-hit != 'true' }}) before installing dependencies in the "install Deps" step is not needed. The subsequent yum install or brew install commands will handle dependency installation regardless of cache status.
    • The mkdir -p /github/workspace/artifacts step is also not necessary if artifacts are downloaded directly to the workspace directory.
  4. Improved artifact handling: After unzipping the workspace artifact, you can utilize relative paths to navigate to specific directories within the extracted workspace structure. It's preferable to avoid hardcoding absolute paths (/github/workspace/artifacts/home/runner/work/pika/pika/) for better flexibility and maintainability.

  5. Diagnostic logging: Adding more diagnostic logging, such as printing the current working directory and listing files (echo pwd, echo `ls```), helps with troubleshooting and verifying the correct directories contain the expected files.

  6. Unit testing enhancement: Consider providing more meaningful output for the unit tests. Currently, it only runs the sh pikatests.sh all command, but adding assertions or capturing the test results would provide better insight into test outcomes.

  7. YAML indentation: Ensure proper indentation in the YAML file to maintain readability and avoid potential syntax errors.

  8. Dependency management improvement: Consider using package managers such as pipenv or poetry to manage and install project dependencies, rather than directly running pip or using system package managers like yum or brew.

Remember to thoroughly test the modified workflow after making changes.

ls -la # Optional: List files in the directory for verification
chmod +x pikatests.sh
sh pikatests.sh all

build_on_macos:
runs-on: macos-latest

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 includes several improvements and bug fixes. Here are some suggestions for further improvement:

  1. Use more descriptive names for jobs: The job names should reflect their purpose or the specific platform being used (e.g., build_on_ubuntu and build_on_macos).

  2. Check for errors during the execution of commands: It's a good practice to add error handling for individual commands and steps in case they fail. This can prevent failures from propagating further in the workflow.

  3. Consider using environment variables instead of hardcoding paths: Instead of hardcoded paths like /home/runner/work/pika/pika/build, you could use environment variables provided by the GitHub Actions runtime. For example, $GITHUB_WORKSPACE represents the path of the workspace directory.

  4. Avoid unnecessary steps: Some steps may not be essential or can be combined if they serve a similar purpose. Review each step to ensure clarity and efficiency.

  5. Ensure consistent indentation: Make sure the indentation is consistent throughout the code to improve readability and avoid accidental errors.

  6. Add comments for clarity: Add comments to explain the purpose and functionality of particular steps or sections to aid understanding for future readers and maintainers.

  7. Consider isolating unit tests in a separate job: It might be beneficial to separate the unit test step into its own job to isolate and parallelize testing if feasible.

  8. Verify expected file presence and contents: After unzipping the workspace artifact, consider adding checks to verify that the expected files are present and have the correct content before performing subsequent actions.

Remember, these suggestions are based on a superficial review of the code patch. It's always important to thoroughly test the changes and consider the specific requirements and constraints of your project.

echo "--------------------"
ls -la # Optional: List files in the directory for verification
chmod +x pikatests.sh
sh pikatests.sh all

build_on_macos:
runs-on: macos-latest
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 code review suggestions for the given code patch:

  1. It seems that a new job called build_on_ubuntu has been added, while the existing build job is commented out. Make sure this change aligns with your intended workflow.

  2. In the Upload workspace artifact step, you can consider specifying a different path for the uploaded artifact to avoid potential conflicts with existing files or directories. For example, instead of using the root directory, you can specify a subdirectory within the artifact space.

  3. The commented-out Test step can be re-enabled and modified to execute the desired test command after the build. Adjust the commands to match your project's test setup.

  4. In the Download workspace artifact step, you may want to consider providing a specific version or using a unique name for the downloaded artifact to avoid using conflicting or outdated artifacts.

  5. If the symbolic link creation in the Create build soft link step is necessary for subsequent steps, make sure it points to the correct target location. Verify the paths and adjust accordingly.

  6. In the Unzip workspace directory step, consider specifying a destination directory other than /github/workspace/artifacts. Using a dedicated directory for unzipping might prevent conflicts with existing files or directories.

  7. In the Unit Test step, make sure that the paths and directories accessed by the commands are accurate and point to the correct locations. Double-check the paths and adjust if needed.

  8. Ensure that all required dependencies and packages are installed properly in each platform-specific block (run) based on your project's needs.

Remember to thoroughly test the workflow after implementing these changes to ensure everything works as expected.

echo "--------------------"
ls -la # Optional: List files in the directory for verification
chmod +x pikatests.sh
sh pikatests.sh all

build_on_macos:
runs-on: macos-latest
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 bug risks and improvement suggestions for the code patch:

  1. In the step Zip workspace directory, the command should include the -q flag to run the zip command quietly:

    run: zip -r -q workspace.zip ${{ github.workspace }}
    
  2. In the step install Deps, ensure that the script pikatests.sh is executable before running it by adding the following command before sh pikatests.sh all:

    chmod +x pikatests.sh
    
  3. In the commented-out Test step, make sure to uncomment it if you want to execute the tests defined by the CMake configuration.

  4. In the step Create artifact directory, change the path to:

    run: mkdir -p /github/workspace/artifacts/pika/pika/build
    
  5. In the step Download workspace artifact, specify the correct name of the artifact to download:

    with:
      name: workspace-artifact
    
  6. In the step 创建 build 软链接, change the command as follows:

    run: ln -s /github/workspace/artifacts/pika/pika/build /github/workspace/artifacts/home/runner/work/pika/pika/build
    
  7. The step Unzip workspace directory should use the correct artifact path:

    run: unzip -q /github/workspace/workspace-artifact.zip -d /github/workspace/artifacts
    
  8. In the step Unit Test, change the command to navigate to the correct directory before running the tests:

    cd /github/workspace/artifacts/pika/pika/
    

These are suggestions based on the provided code, but please note that a more thorough review may require understanding the overall context and requirements of your project.

cd /github/workspace/artifacts/home/runner/work/pika/pika/build
python3 ../tests/integration/pika_replication_test.py
python3 ../tests/unit/Blpop_Brpop_test.py

build_on_macos:
runs-on: macos-latest

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 there are a few bug risks and improvement suggestions:

  1. In the job name change, consider using a more descriptive name like "build_on_ubuntu" to make it clear that this job is specifically for Ubuntu.

  2. When zipping the workspace directory, make sure to exclude any unnecessary or sensitive files or directories that shouldn't be included in the artifact.

  3. You can improve the organization of the job by separating the steps into logical sections with comments. This makes it easier to understand and maintain the workflow.

  4. Consider using the cache action for caching dependencies to speed up subsequent runs. It appears that the cache step is used for caching pip dependencies, but you could also explore caching other dependencies if applicable.

  5. Make sure to handle error conditions appropriately. For example, check if the artifact download is successful before proceeding with subsequent steps.

  6. Use environment variables instead of hardcoding paths where possible (e.g., /opt/rh/devtoolset-10/enable).

  7. Add error handling and feedback mechanisms for each step in the workflow, so you can easily troubleshoot and identify failures.

  8. Consider using matrix builds to run the same job on different versions of Ubuntu and/or configurations for better coverage. This allows you to test your code on multiple setups efficiently.

These suggestions aim to improve code readability, maintainability, and overall reliability of the workflow. It's important to thoroughly test and validate the workflow changes before applying them to production environments.

cd /github/workspace/artifacts/home/runner/work/pika/pika/build
python3 ../tests/integration/pika_replication_test.py
python3 ../tests/unit/Blpop_Brpop_test.py

build_on_macos:
runs-on: macos-latest

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 to have some improvements and bug fixes. Here are a few suggestions for further improvement:

  1. Provide more descriptive names for the jobs: Instead of build_on_ubuntu and build_on_macos, consider using more meaningful names that describe the purpose of each job. This can help with readability and understanding the workflow.

  2. Add error handling and resilience: It would be beneficial to add error handling and resilience mechanisms in case any command or step fails. For example, you could use try-catch blocks or conditional statements to handle potential failures during artifact downloading or unzipping.

  3. Review dependencies installation: Ensure that all necessary dependencies are correctly listed and required versions are specified. Double-check if there are any additional dependencies required for successful execution.

  4. Simplify paths and directory structure: If possible, avoid using hardcoded paths and create a more flexible and generic directory structure. This can make the code more portable and adaptable to different environments.

  5. Add comments: Although the code is relatively self-explanatory, it's always helpful to add comments explaining the purpose of specific steps or sections to improve code understandability and maintainability.

  6. Consider using matrix builds: If cross-platform coverage is a requirement, consider converting the workflow into a matrix build. This allows running the same set of steps on multiple platforms without duplicating the code.

  7. Test error scenarios: It's essential to test potential error scenarios, such as the failure of artifact download or unexpected behavior in test scripts. Include tests or checks to ensure your workflow handles these situations gracefully.

Remember to thoroughly test the updated workflow to verify its correctness and effectiveness.

cd /github/workspace/artifacts/home/runner/work/pika/pika/build
python3 ../tests/integration/pika_replication_test.py
python3 ../tests/unit/Blpop_Brpop_test.py

build_on_macos:
runs-on: macos-latest

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 there are a few suggestions and improvements that can be made:

  1. Job Name: Instead of renaming the existing build job to build_on_ubuntu, it might be better to create a new job with the desired name and keep the original build job for compatibility reasons.

  2. Artifact Management:

  • The step to zip the entire workspace directory and upload it as an artifact is good. However, you should consider excluding unnecessary files or directories from being included in the zip file.
  • In the subsequent job, you're downloading the artifact and unzipping it. Instead of downloading and unzipping, you can specify the artifact path directly using the path parameter in the Download workspace artifact step. This way, you don't need to manually unzip the workspace.
  • Make sure to cleanup or delete the workspace.zip after artifact upload and extraction, as it might take up unnecessary space.
  1. Dependency Installation:
  • Consider using package managers like apt-get instead of yum in the Ubuntu job. apt-get is commonly used for package management on Ubuntu-based systems.
  • In the CentOS job, make sure to use yum update before installing other dependencies to ensure the system packages are up to date.
  • It's good to upgrade pip before installing dependencies, but you may want to specify a specific version to avoid potential compatibility issues if a major pip version update occurs.
  1. Test Execution:
  • In the Unit Test step, when moving the build directory, ensure that the destination directory doesn't already exist to prevent overwriting any existing data.
  • When running tests, provide the necessary configuration or command line arguments required for successful execution.
  • For the E2E tests, it's recommended to include specific assertions or checks to verify the expected outcomes of the tests.

These suggestions are aimed at improving code clarity and reliability. Remember to thoroughly test the code patch after making these changes to ensure it works as expected.

# cd /github/workspace/artifacts/home/runner/work/pika/pika/build
# python3 ../tests/integration/pika_replication_test.py
# python3 ../tests/unit/Blpop_Brpop_test.py

build_on_macos:
runs-on: macos-latest

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. Job Names: It's a good practice to use descriptive names for jobs. Instead of renaming the existing "build" job, you added a new job named "build_on_ubuntu." Consider giving it a more specific name that reflects its purpose or target platform.

  2. Zip and Artifact Handling: The addition of zipping the workspace directory and uploading it as an artifact is a good improvement. You can simplify the process by using the actions/upload-artifact action directly after the zipping step, without creating a separate job for it. No need to download and unzip it again in subsequent jobs. You can access the artifact directly in subsequent jobs.

  3. Installation of Dependencies: You're installing additional packages (which, tcl, unzip, python3) and upgrading pip. These steps seem fine, but ensure that these dependencies are necessary for your project and version compatibility.

  4. Caching Dependencies: Caching installed dependencies can improve build times. Make sure the caching mechanism works correctly by testing it thoroughly.

  5. Improving Readability: Add comments to clarify the purpose of each step, job, or group of commands. This helps other developers (and even yourself) understand the code better.

  6. Unit Tests and Integration Tests: There seems to be a section commented out that appears to handle unit tests and integration tests. Depending on what you're aiming for, you might want to uncomment and define the usage of these tests explicitly.

  7. Remove Unused Section: The last empty section (build_on_macos) does not contain any steps or configurations. You can remove it from the code snippet.

It's important to note that a complete code review would require analyzing the context and purpose of the entire workflow where this code patch is being integrated. This assessment is based solely on the provided code patch.

@@ -813,7 +813,7 @@ def rpush_thread(list_, value_, value2_, value3_, value4_, value5_):

master.close()
slave.close()
print("test_master_slave_replication Passed [], db:db%d" % (db_))
print("test_master_slave_replication Passed [Passed], db:db%d" % (db_))

def test_with_db(db_id):
test_master_slave_replication(db_id)
Copy link

Choose a reason for hiding this comment

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

Here are a few suggestions to improve the code:

  1. Use descriptive variable names: Instead of using single-letter variable names like db_, list_, and value_, consider using more descriptive names such as database_number, list_name, and item_value. This will make the code easier to understand and maintain.

  2. Remove unnecessary comments: Comments like # 解阻塞测试(超时自动解阻塞,lpush解阻塞,rpush解阻塞,rpoplpush解阻塞) can be removed or translated into English for better readability, depending on your target audience.

  3. Consistent naming convention: Make sure to use consistent naming conventions throughout the code. For example, you have both test_blpop_brpop_unblock_lrpush_rpoplpush and brpop_thread51. It's good practice to follow a naming convention such as snake_case for function and variable names to improve code readability.

  4. Use f-strings for formatting: Instead of using string concatenation with % operator, consider using f-strings for better readability and simpler syntax. For example, print("test_single_existing_list Passed [Passed], db:db%d" % (db_)) can be written as print(f"test_single_existing_list Passed [Passed], db:db{db_}").

  5. Avoid hardcoded values: In functions like rpush_thread and rpush_thread2, instead of hardcoding the list names ('blist0', 'blist1', 'blist2', 'blist3'), consider passing them as arguments to the function. This will make the code more flexible and reusable.

  6. Add exception handling: Consider adding appropriate exception handling throughout the code to handle potential errors or exceptions that may occur during execution. This will make the code more robust.

Overall, the code appears to be mostly focused on printing some success messages and assertions. Without a complete understanding of the context and requirements, it's difficult to identify specific bug risks or suggest further improvements. It's recommended to thoroughly test the code with different scenarios to ensure its correctness and efficiency.

@@ -813,7 +813,7 @@ def rpush_thread(list_, value_, value2_, value3_, value4_, value5_):

master.close()
slave.close()
print("test_master_slave_replication Passed [], db:db%d" % (db_))
print("test_master_slave_replication Passed [Passed], db:db%d" % (db_))

def test_with_db(db_id):
test_master_slave_replication(db_id)
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 to be mostly focused on modifying print statements to change the output messages. There don't appear to be any bug risks or improvement suggestions based on the provided context.

However, here are a few general good practices for code review:

  1. Use meaningful variable and function names: Ensure that variable and function names accurately describe their purpose or functionality. This improves code readability and maintainability.

  2. Add comments: Consider adding comments to explain complex sections of code or provide additional context where necessary. This can make it easier for others (and even yourself) to understand the code later on.

  3. Maintain consistent code style: Consistent indentation, spacing, and formatting improve the code's readability. Ensure that the code adheres to a specific style guide or convention consistently throughout.

  4. Check error handling: Verify if error handling mechanisms (like try-except blocks) are implemented correctly and handle potential exceptions gracefully to prevent unexpected crashes or behavior.

Remember, without the full context of the code and its purpose, it's difficult to provide specific improvements or identify potential bugs. It's always a good practice to thoroughly test your code with different scenarios to ensure its correctness and reliability.

@@ -813,7 +813,7 @@ def rpush_thread(list_, value_, value2_, value3_, value4_, value5_):

master.close()
slave.close()
print("test_master_slave_replication Passed [], db:db%d" % (db_))
print("test_master_slave_replication Passed [Passed], db:db%d" % (db_))

def test_with_db(db_id):
test_master_slave_replication(db_id)
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. The changes made in the patch are purely cosmetic, updating the print statements to change "[✓]" to "[Passed]". These changes don't affect the functionality or correctness of the code.

  2. It's recommended to add more comments or docstrings to explain the purpose of each function and any significant logic within the code. This will make it easier for future developers to understand and maintain the code.

  3. Consider using a logging library instead of print statements for better log management and flexibility. The logging module in Python provides a more robust and configurable way to handle logging.

  4. Reviewing only this code patch without seeing the rest of the codebase limits the potential identification of bugs and improvements. For a more comprehensive review, it would be helpful to inspect the entire codebase.

@@ -813,7 +813,7 @@ def rpush_thread(list_, value_, value2_, value3_, value4_, value5_):

master.close()
slave.close()
print("test_master_slave_replication Passed [], db:db%d" % (db_))
print("test_master_slave_replication Passed [Passed], db:db%d" % (db_))

def test_with_db(db_id):
test_master_slave_replication(db_id)
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 are some potential bug risks and improvement suggestions:

  1. In the test_single_existing_list function, the assert statement comparing the expected and actual results could be written in a more concise way using tuple unpacking:

    assert result == (b'blist', b'b'), f"Expected (b'blist', b'b'), but got {result}"
    
  2. In the brpop_thread51 function, there is no need for the additional assert statement after joining the thread. You can safely remove it.

  3. In the rpush_thread function, you should consistentl

Copy link
Contributor

@yaoyinnan yaoyinnan left a comment

Choose a reason for hiding this comment

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

lgtm

@Mixficsol Mixficsol merged commit 2597ac3 into OpenAtomFoundation:unstable Jul 21, 2023
9 checks passed
@Mixficsol Mixficsol deleted the atom_centos branch July 21, 2023 15:58
bigdaronlee163 pushed a commit to bigdaronlee163/pika that referenced this pull request Jun 8, 2024
* support centos on ci

* add unit test on centos

* find path

* fix PythonE2E on centos
cheniujh pushed a commit to cheniujh/pika that referenced this pull request Sep 24, 2024
* support centos on ci

* add unit test on centos

* find path

* fix PythonE2E on centos
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants