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(packages): add offline pkg scripts #189

Merged
merged 1 commit into from
Dec 29, 2023

Conversation

wuhuizuo
Copy link
Contributor

Signed-off-by: wuhuizuo [email protected]

@ti-chi-bot ti-chi-bot bot requested review from jayl1e and purelind December 29, 2023 04:37
@ti-chi-bot ti-chi-bot bot added the size/XL label Dec 29, 2023
Copy link

ti-chi-bot bot commented Dec 29, 2023

I have already done a preliminary review for you, and I hope to help you do a better job.

Pull Request Review

Title

The title provides a brief summary of the changes, which is the addition of offline package scripts.

Description

The description is empty, aside from a signature by the author. It would be beneficial to include a verbose explanation of what the changes are and why they were made.

Changes

The changes in the pull request can be summarized as follows:

  1. Modifications to offline-packages.yaml.tmpl:

    • Reorganized and expanded the structure to include editions and artifactory fields.
    • Added tags to package_repo to differentiate between community and enterprise versions.
    • Added a script to generate offline package artifacts for different versions, operating systems, and architectures.
  2. Deletion of compose-offline-packages-artifact.sh.tmpl file:

    • This script was removed, and it appears its functionality has been replaced by the new scripts added.
  3. Addition of compose-offline-packages-artifacts.sh.tmpl file:

    • This new script seems to replace the deleted compose-offline-packages-artifact.sh.tmpl.
  4. Addition of gen-package-offline-package-with-config.sh file:

    • This script generates package build scripts for different target configurations.

Potential Problems

  1. The changes lack documentation. It is recommended to include comments in the code explaining the purpose and functionality of the new scripts.

  2. No tests have been included or updated to cover the new scripts and changes.

Suggestions

  1. Add a more comprehensive description to the PR, detailing the reasons for the changes and their expected impact.
  2. Include comments in the code to explain the purpose and functionality of the new scripts.
  3. Include tests to cover the new scripts and changes, ensuring they work as expected.
  4. Verify that the removal of the compose-offline-packages-artifact.sh.tmpl file does not break any dependencies or expected functionalities.

Markdown

# Pull Request Review

## Title

The title provides a brief summary of the changes, which is the addition of offline package scripts.

## Description

The description is empty, aside from a signature by the author. It would be beneficial to include a verbose explanation of what the changes are and why they were made.

## Changes

The pull request includes the following key changes:

1. Modifications to `offline-packages.yaml.tmpl`:
   - Reorganization and expansion of the structure to include `editions` and `artifactory` fields.
   - Addition of `tags` to `package_repo` to differentiate between community and enterprise versions.
   - Addition of a script to generate offline package artifacts for different versions, operating systems, and architectures.
   
2. Deletion of `compose-offline-packages-artifact.sh.tmpl` file:
   - This script was removed, and it appears its functionality has been replaced by the new scripts added.

3. Addition of `compose-offline-packages-artifacts.sh.tmpl` file:
   - This new script seems to replace the deleted `compose-offline-packages-artifact.sh.tmpl`.

4. Addition of `gen-package-offline-package-with-config.sh` file:
   - This script generates package build scripts for different target configurations.

## Potential Problems

1. The changes lack documentation. It is advisable to include comments in the code explaining the purpose and functionality of the new scripts.

2. No tests have been included or updated to cover the new scripts and changes.

## Suggestions

1. Add a more comprehensive description to the PR, detailing the reasons for the changes and their expected impact.
2. Include comments in the code to explain the purpose and functionality of the new scripts.
3. Include tests to cover the new scripts and changes, ensuring they work as expected.
4. Verify that the removal of the `compose-offline-packages-artifact.sh.tmpl` file does not break any dependencies or expected functionalities.

@wuhuizuo wuhuizuo force-pushed the feature/offline-pkg-scripts branch from e9d5503 to 50533a4 Compare December 29, 2023 05:42
Copy link

ti-chi-bot bot commented Dec 29, 2023

I have already done a preliminary review for you, and I hope to help you do a better job.

Pull Request Review

Key Changes

  1. The offline-packages.yaml.tmpl file has been updated with new configurations for community and enterprise editions. The artifactory and artifacts blocks have been moved inside these configurations.

  2. A new bash script test_gen_offline_package_artifacts_script has been added to ci.sh file to test the generation of offline package artifacts.

  3. The compose-offline-packages-artifact.sh.tmpl file has been deleted.

  4. A new compose-offline-packages-artifacts.sh.tmpl file has been created which contains a main function to compose and optionally push artifacts. Functions to handle argument parsing, help message, and composing packages have been added.

  5. A new gen-package-offline-package-with-config.sh file has been created that sets up the environment for generating package build scripts.

Potential Problems

  1. The deletion of the compose-offline-packages-artifact.sh.tmpl file could lead to problems if any other parts of the codebase were reliant on it.

  2. The new compose-offline-packages-artifacts.sh.tmpl script has complex logic that could be prone to errors.

  3. In ci.sh, the test function test_gen_offline_package_artifacts_script has hardcoded values for versions, operating systems, architectures and editions. This could lead to issues in future if these values need to change or be expanded.

  4. The pull request lacks a detailed description, making it harder to understand the rationale behind these changes.

Suggestions for Fixes

  1. Ensure that the deletion of compose-offline-packages-artifact.sh.tmpl does not impact other parts of the system.

  2. Thoroughly test the new compose-offline-packages-artifacts.sh.tmpl script to ensure it functions as expected.

  3. Consider refactoring the test_gen_offline_package_artifacts_script function in ci.sh to make it more flexible and maintainable, for example by using configuration files or command line arguments instead of hardcoded values.

  4. Provide a more detailed description in the pull request to help reviewers understand the changes and their purpose.

@wuhuizuo wuhuizuo force-pushed the feature/offline-pkg-scripts branch from 50533a4 to efd883f Compare December 29, 2023 05:43
Copy link

ti-chi-bot bot commented Dec 29, 2023

I have already done a preliminary review for you, and I hope to help you do a better job.

Summary

This pull request is titled "feat(packages): add offline pkg scripts" and mainly modifies the following parts.

  1. It adds a new YAML template for offline packages (packages/offline-packages.yaml.tmpl). This new file seems to be used for defining the offline packages for a software. It includes details like package repository, tags, routers, etc. for community and enterprise editions.

  2. It removes two scripts (packages/scripts/build-offline-package-with-config.sh and packages/scripts/compose-offline-packages-artifact.sh.tmpl), and adds two new scripts (packages/scripts/gen-package-offline-package-with-config.sh and packages/scripts/compose-offline-packages-artifacts.sh.tmpl). These scripts seem to be used for generating and composing offline packages.

  3. It modifies the CI script (packages/scripts/ci.sh) to add a new test function for testing the generation of offline package artifacts script.

Potential Problems

  1. The removed scripts may still be in use somewhere, which may cause compatibility issues.

  2. There is no detailed description provided for the PR. It's hard to understand the purpose of these changes without any context.

  3. The file packages/offline-packages.yaml.tmpl seems to be a template file, but it's not clear how it will be used and filled with actual data.

  4. The CI script is modified to add testing for the new script, but it's not clear if these tests are comprehensive enough.

  5. The new scripts added are quite complex and may need more thorough reviews to ensure they work as expected.

Suggestions

  1. The contributor should provide a detailed PR description to explain the purpose and context of these changes.

  2. It would be helpful to explain how the new YAML template file will be used.

  3. The removed scripts should be checked to ensure they are not used anywhere else in the project.

  4. The new scripts should be reviewed in detail to ensure they work correctly and are error-free.

  5. The test coverage for the new scripts should be verified.

Here is an example of how to provide feedback in markdown format:

## PR Review

- The PR lacks a detailed description of the changes made. Please provide more context about this feature.
- The `packages/offline-packages.yaml.tmpl` file seems to be a new template for defining offline packages. Could you explain more about how it is used?
- Two scripts have been removed. Ensure they are not in use anywhere else in the project to avoid compatibility issues.
- Two new scripts have been added. They should be reviewed thoroughly to ensure their correctness.
- The CI script has been updated to include a new test for the script that generates offline package artifacts. Verify that these tests are comprehensive.

@wuhuizuo
Copy link
Contributor Author

/approve

Copy link

ti-chi-bot bot commented Dec 29, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wuhuizuo

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot bot added the approved label Dec 29, 2023
@ti-chi-bot ti-chi-bot bot merged commit 7c28d60 into main Dec 29, 2023
1 of 2 checks passed
@ti-chi-bot ti-chi-bot bot deleted the feature/offline-pkg-scripts branch December 29, 2023 06:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant