refactor(packages): split builder from routers

[wuhuizuo]

• routers control the artifacts changes
• builders control the builder changes.

Signed-off-by: wuhuizuo [email protected]

[ti-chi-bot]

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

Review Summary

The pull request titled refactor(packages): split builder from routers brings several changes to the codebase, primarily focused on refactoring the structure of the package configuration and its related scripts.

Key Changes

1. The 'builder' field is removed from the routers in the package configuration (packages.yaml.tmpl), and a new 'builders' field is introduced separately. This provides a cleaner separation between routing and building logic.
2. The builder image is now conditionally selected based on the release version. This could be helpful in managing different builder versions for different release versions.
3. The scripts gen-package-artifacts-with-config.sh, gen-package-images-with-config.sh, and get-package-builder-with-config.sh have been updated to reflect these changes.

Potential Problems

1. Version Constraints: The conditionals for selecting builder images are based on the semver constraints like >=7.1.0-0 and ~6.5.6-0. This requires that the .Release.version be correctly formatted and versioned, or else the builder selection may not work correctly.

2. Error Handling: In the scripts gen-package-artifacts-with-config.sh and gen-package-images-with-config.sh, the exit status for unmatched routes has been changed from 0 to 1, indicating an error. This change might affect other scripts or processes that rely on these scripts.

3. In the script get-package-builder-with-config.sh, the builder is selected based on the first match. If there are multiple builders satisfying the condition, only the first one will be used.

Suggested Fixes

1. Ensure that .Release.version is correctly versioned according to semver, possibly by adding validation checks.

2. Review any dependencies on the scripts gen-package-artifacts-with-config.sh and gen-package-images-with-config.sh to ensure they can handle an exit status of 1 for unmatched routes.

3. Consider whether it would be more appropriate to iterate through all matching builders, or explicitly handle the case where multiple builders match the conditions.

# Code Review Changes
- Ensure `.Release.version` is correctly versioned according to semver.
- Review dependencies on scripts `gen-package-artifacts-with-config.sh` and `gen-package-images-with-config.sh` to handle an exit status of 1 for unmatched routes.
- Consider iterating through all matching builders in `get-package-builder-with-config.sh`, or explicitly handle the case where multiple builders match the conditions.

[ti-chi-bot]

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

Review Summary

This pull request primarily refactors the build and routing process of the package. The key changes include:

1. The builder is separated from the routers. The builder details are now defined under the builders section rather than being part of the routers section. This simplifies and separates the responsibilities of builders and routers.

2. The builder selection has been moved to be based on the semver.CheckConstraint function, which checks the package version and determines the appropriate builder.

3. Some changes are made in the scripts gen-package-artifacts-with-config.sh, gen-package-images-with-config.sh and get-package-builder-with-config.sh to accommodate the changes in the configuration file structure.

Potential Issues

1. Builder Definition: The builder images are now determined by version constraints. This could potentially lead to issues if the version constraint logic is not accurate.

2. Error Handling: The scripts now exit with an error status (exit 1) when no matching routes or builders are found. This may affect the workflow if there are scenarios where having no routes/builders is a valid case.

3. Backward Compatibility: The changes are quite significant and might not be backward compatible. If there are existing systems or scripts relying on the old configuration structure, this could cause issues.

Suggestions

1. Test Version Constraints: It is recommended to thoroughly test the version constraints to ensure the right builder is selected for the right version.

2. Handle Errors Gracefully: Consider if there's a better way to handle scenarios where no routes or builders are found, rather than just exiting with an error.

3. Ensure Backward Compatibility: If backward compatibility is a concern, consider maintaining a legacy version of the scripts/config structure or provide a migration path for systems that are using the old structure.

[ti-chi-bot]

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

Review

Summary

This pull request primarily focuses on refactoring the package configuration in packages/packages.yaml.tmpl and modifying the scripts in packages/scripts/ to accommodate these changes. The changes broadly fall into the following categories:

• Addition of builder configurations for different components based on version ranges in packages.yaml.tmpl.
• Modification of router configurations in packages.yaml.tmpl to accommodate the new builder configurations.
• Changes in .github/workflows/pull-verify-packages-config.yaml to add testing for more versions and components.
• Changes in the scripts gen-package-artifacts-with-config.sh, gen-package-images-with-config.sh, and get-package-builder-with-config.sh to filter builders and handle error cases properly.

Potential Problems

1. Unverified Builder Images: The pull request introduces multiple new builder images for different components and version ranges. These images need to be validated to ensure that they are correct and functional.

2. Complex Versioning Logic: The builder and router configurations are now dependent on complex versioning logic. This may be hard to maintain and could lead to potential errors if not thoroughly tested.

3. Inconsistency in Error Handling: In the scripts gen-package-artifacts-with-config.sh and gen-package-images-with-config.sh, we can see inconsistent error handling. In the former, if no routes are matched, it logs an error message and exits with a status of 1, whereas in the latter, it only exits with a status of 0.

Suggestions

1. Verify Builder Images: Add tests or checks to verify that the new builder images are working as expected.

2. Refactor Versioning Logic: Consider refactoring the versioning logic to make it simpler and easier to maintain. This could potentially involve breaking down the complex versioning logic into smaller, more manageable parts or creating helper functions to handle the version comparisons.

3. Consistent Error Handling: Make error handling consistent across scripts. In the case where no routes are matched, both scripts should behave consistently, either by logging an error message and exiting with a status of 1, or simply exiting with a status of 0.

[ti-chi-bot]

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

Review for Pull Request: Refactor Packages - Split Builder From Routers

Summary of Changes

The pull request makes several changes related to the configuration of the package building process. These changes include:

1. Splitting the builder and router logic within the GitHub workflow configuration.
2. Adding the builder image configuration to the package YAML template.
3. Modifying the scripts involved in the package building process.

The detailed changes are as follows:

.github/workflows/pull-verify-packages-config.yaml

• The 'advanced-statefulset' was added to the test scripts.
• The 'ctl' component was added to the component list for testing.
• Testing now includes versions v6.5.0 and v6.1.0.

packages/packages.yaml.tmpl

• Introduced a new 'builders' field to the components, which specifies the builder images for different versions.
• The 'builder' field was removed from the 'routers' field.
• Additional versions and conditions were added to the 'routers' field.

packages/scripts/gen-package-artifacts-with-config.sh, packages/scripts/gen-package-images-with-config.sh, and packages/scripts/get-package-builder-with-config.sh

• Adjustments were made to these scripts to accommodate the splitting of builder and router configuration.

Potential Problems

The changes introduced in this PR are extensive and touch upon several aspects of the package building process. It might increase the risk of introducing breaking changes. Below are some potential problems:

1. Compatibility Issues: The changes in the builder images and the addition of new components and versions might cause compatibility issues with the existing codebase.
2. Testing Issues: The changes in the testing scripts and workflow configuration might cause the tests to fail or produce incorrect results.
3. Configuration Errors: Changes in the package YAML template could lead to configuration errors if not properly implemented or if there are typos or syntax errors.

Suggestions for Improvement

1. Extensive Testing: Given the extensive changes made in this PR, it's crucial to thoroughly test the changes to ensure they work as expected and do not introduce new bugs.
2. Incremental Changes: Instead of making all changes in a single PR, consider making them incrementally. This way, it's easier to identify the source of problems if they occur.
3. Code Review: Have the changes reviewed by multiple developers to catch any potential issues or errors.
4. Error Handling: Ensure that proper error handling is in place, especially for the changes in the scripts, so that issues can be caught and resolved quickly.

Please consider the above points and make the necessary changes or improvements.

[ti-chi-bot]

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

Summary

This pull request appears to be refactoring the structure of packages by separating the builder from routers. The changes seem to be focused on simplifying and enhancing the configuration of the packages by adding conditions to select the correct builder image based on the version of the release.

Key Changes:

1. The builder selection process has been moved from routers to a separate builders section in the packages.yaml.tmpl file, and conditions have been added to select the correct builder image based on the release version.
2. Multiple checks have been added to ensure that only one builder is matched. If no builder or more than one builder is matched, an error is thrown.
3. The scripts gen-package-artifacts-with-config.sh, gen-package-images-with-config.sh, and get-package-builder-with-config.sh have been updated to reflect the changes in the packages.yaml.tmpl file.
4. Some routes in packages.yaml.tmpl file have been updated or added to match more version ranges.

Potential Problems

1. The changes in the scripts and the packages.yaml.tmpl file are significant. If there are any mistakes in the condition checks or the builder selection process, it could potentially select the wrong builder or throw unnecessary errors.
2. The changes are adding complexity to the configuration. This could make it harder to maintain and understand the configuration in the future.

Suggestions

1. Please ensure to test the changes thoroughly to make sure the correct builder is selected in all scenarios and no unnecessary errors are being thrown.
2. Comments could be added to explain the more complex parts of the configuration, especially the parts that involve condition checks.
3. As the changes are quite significant, it might be beneficial to have a second reviewer to go through the pull request.

[ti-chi-bot]

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

Review

The Pull Request (PR) is a refactor that splits the builder from routers. It modifies several files, introduces a new shell script (ci.sh), and changes the way builders and routers are configured and handled in the CI/CD process.

Key Changes:

1. The refactoring process involves updating the existing CI/CD scripts and separating the concerns of builders and routers. The builders are responsible for building the software, while the routers control the artifacts changes.
2. The script ci.sh is added to the packages/scripts directory. The script contains functions to test the builder configuration, generate package artifacts, and generate package images.
3. The PR removes a large amount of code from the .github/workflows/pull-verify-packages-config.yaml file and replaces it with a call to the new ci.sh script.
4. The packages/packages.yaml.tmpl has been extensively modified to adjust the builder and router configurations for various components and versions.

Potential Problems:

1. There are no unit tests or integration tests included in the PR to validate the changes in the scripts and the CI/CD pipeline.
2. The new script ci.sh does not have any error handling or logging mechanism.
3. If there are any existing dependencies on the old configuration of builders and routers, they could potentially break after these changes.
4. The PR does not provide a detailed explanation of why these changes are necessary and what problems they are solving.

Fixing Suggestions:

1. Add unit tests or integration tests to validate the changes in the scripts and the CI/CD pipeline.
2. Include error handling and logging in ci.sh for easier debugging and tracing.
3. Provide a detailed explanation in the PR description about why these changes are necessary, how they were tested, and what problems they solve.
4. Test these changes in a controlled or staging environment first to ensure that they don't break the existing CI/CD pipeline or any dependencies.

# Requested Changes

Thank you for your PR. Here are some requested changes:

1. Please add unit tests or integration tests to validate the changes in the scripts and the CI/CD pipeline.
2. Please include error handling and logging in `ci.sh` for easier debugging and tracing.
3. Please update the PR description to provide a detailed explanation about why these changes are necessary, how they were tested, and what problems they solve.
4. Please ensure these changes are tested in a controlled or staging environment first to ensure that they don't break the existing CI/CD pipeline or any dependencies.

Once these changes are made, I'd be happy to review your PR again.

[ti-chi-bot]

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

Review

The PR titled "refactor(packages): split builder from routers" primarily aims to separate the builder logic from the routers in the GitHub CI workflows.

Changes

1. The script in the .github/workflows/pull-verify-packages-config.yaml file has been largely removed and replaced by a simpler one that runs a new script ci.sh.
2. A new script ci.sh has been added under packages/scripts/ directory.
3. The packages/packages.yaml.tmpl file has been heavily refactored. The primary changes include separating the builders from the routers, adding version constraints for using different go versions, and removing the builder field from the router sections.
4. Changes have been made to the scripts gen-package-artifacts-with-config.sh, gen-package-images-with-config.sh, and get-package-builder-with-config.sh under packages/scripts/ directory to accommodate the changes made in packages/packages.yaml.tmpl.

Potential Issues

1. The changes have been made assuming that the new ci.sh script works as expected. However, there seems to be no tests or checks to confirm the same.
2. The new builders have been added based on some version constraints. However, the correctness of these constraints has not been verified.
3. The version constraints for using different go versions have been added in the packages/packages.yaml.tmpl. However, the correctness of these constraints has not been verified.

Suggestions

1. Add tests or checks to verify the correctness of the new ci.sh script.
2. Verify the correctness of the version constraints for the builders.
3. Verify the correctness of the version constraints for using different go versions.

### Review Summary

The PR titled "refactor(packages): split builder from routers" primarily aims to separate the builder logic from the routers for the GitHub CI workflows.

#### Key Changes

1. The script in the `.github/workflows/pull-verify-packages-config.yaml` file has been largely removed and replaced by a simpler one that runs a new script `ci.sh`.
2. A new script `ci.sh` has been added under `packages/scripts/` directory.
3. The `packages/packages.yaml.tmpl` file has been heavily refactored. The primary changes include separating the builders from the routers, adding version constraints for using different go versions, and removing the `builder` field from the `router` sections.

#### Potential Issues

1. The changes have been made assuming that the new `ci.sh` script works as expected. However, there seems to be no tests or checks to confirm the same.
2. The new builders have been added based on some version constraints. However, the correctness of these constraints has not been verified.

#### Suggestions for Improvement

1. Add tests or checks to verify the correctness of the new `ci.sh` script.
2. Verify the correctness of the version constraints for the builders.

[ti-chi-bot]

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

Review

Summary

This Pull Request (PR) is titled refactor(packages): split builder from routers. It primarily aims to refactor the codebase by separating the builder from the routers in the packages.

The commit is signed by wuhuizuo, suggesting that the author stands by the changes made in this PR.

The key changes in the PR are in the GitHub workflow file (pull-verify-packages-config.yaml), the package definitions template file (packages.yaml.tmpl), and a set of shell scripts (gen-package-artifacts-with-config.sh, gen-package-images-with-config.sh, get-package-builder-with-config.sh).

Additionally, a new shell script file ci.sh was added.

Potential Problems

1. Testing

It's not clear if the changes were adequately tested. Although a ci.sh script has been added, there is no explicit mention of tests being run on the new code.

2. Code complexity

This PR introduces a significant amount of complexity, with many nested conditions and loops. This could make the codebase more difficult to maintain and debug in the future.

3. Lack of context

The PR lacks detailed context in the description to explain the rationale behind the changes made. This makes it challenging for reviewers to assess the impact of the changes.

Suggestions for Improvement

1. Testing

Ensure that there are tests covering the changes and all tests are passing.

2. Code complexity

Consider refactoring the code to reduce complexity. Extracting common code into functions and keeping the condition logic simple can help.

3. Provide more context

Add more information to the PR description explaining the motivation for the changes, the problems this PR aims to solve, and how the changes address these problems.

Comments to the author

Thanks for your effort in this PR. The changes look substantial and likely required a lot of work. 

Here are a few questions and suggestions:

1. Could you provide more context about why these changes are necessary? It will help reviewers understand the intent behind this PR.
2. Have these changes been tested? If so, could you share the results? If not, please consider adding tests to ensure the changes work as expected.
3. The PR introduces a significant amount of complexity. Have you considered alternative approaches that could achieve the same result with less complexity?

Looking forward to your response.

[ti-chi-bot]

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

Review for the Pull Request

Summary of changes

• The pull request (PR) refactors the package configuration by separating the builder from router.
• The PR introduces a new script, ci.sh, which can be used to test the get-package-builder-with-config.sh, gen-package-artifacts-with-config.sh, and gen-package-images-with-config.sh scripts. It is suggested to run this locally before committing.

Potential problems

• The gen-package-artifacts-with-config.sh and gen-package-images-with-config.sh scripts were modified to exit with a non-zero code if no routes match. This could potentially cause a pipeline to fail if these scripts are used in a CI/CD pipeline.
• Some versions of the builder images are hard-coded in the package configuration. If these images are not available, the scripts using them may fail.

Fixing suggestions

• Instead of exiting with a non-zero code when no routes match, consider logging a warning and exiting with a zero code. This would prevent a pipeline from failing, but still alert the user to the potential issue.
• Consider using a configuration file or environment variables to specify the versions of the builder images. This would allow the versions to be changed easily without modifying the scripts.

Markdown format review

# Review for the Pull Request

## Summary of changes

The pull request (PR) refactors the package configuration by separating the `builder` from `router`.
The PR introduces a new script, `ci.sh`, which can be used to test the `get-package-builder-with-config.sh`, `gen-package-artifacts-with-config.sh`, and `gen-package-images-with-config.sh` scripts. It is suggested to run this locally before committing.

## Potential problems

1. The `gen-package-artifacts-with-config.sh` and `gen-package-images-with-config.sh` scripts were modified to exit with a non-zero code if no routes match. This could potentially cause a pipeline to fail if these scripts are used in a CI/CD pipeline.
2. Some versions of the `builder` images are hard-coded in the package configuration. If these images are not available, the scripts using them may fail.

## Fixing suggestions

- Instead of exiting with a non-zero code when no routes match, consider logging a warning and exiting with a zero code. This would prevent a pipeline from failing, but still alert the user to the potential issue.
- Consider using a configuration file or environment variables to specify the versions of the `builder` images. This would allow the versions to be changed easily without modifying the scripts.

[wuhuizuo]

/approve

[ti-chi-bot]

[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:

• OWNERS [wuhuizuo]

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