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 builder for ng-monitoring #176

Merged
merged 2 commits into from
Dec 18, 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 18, 2023 09:26
Copy link

ti-chi-bot bot commented Dec 18, 2023

I have already done a preliminary review for you, and I hope to help you do a better job.
The pull request adds a builder for ng-monitoring. The changes are made in the packages.yaml.tmpl file. The builder field has been added under the steps section for release and fips.

There are no potential problems in this pull request. However, it would be best if the pull request description provided more information about the changes made.

As for fixing suggestions, if the pull request description can provide more information, it will be easier for other developers to understand the implementation details and make future contributions.

@ti-chi-bot ti-chi-bot bot added the size/XS label Dec 18, 2023
Copy link

ti-chi-bot bot commented Dec 18, 2023

I have already done a preliminary review for you, and I hope to help you do a better job.
Overall, this pull request adds a builder for ng-monitoring and also simplifies the existing test script by removing unnecessary loops.

There are no major problems with this pull request, but here are some minor suggestions for improvement:

  1. The pull request description is too brief. It would be helpful to have more context about why this change is being made.

  2. In the packages.yaml.tmpl file, there are inconsistent uses of whitespace. It would be better to have a consistent formatting style throughout the file.

  3. The builder field in the packages.yaml.tmpl file is hard-coded with a specific version number. It might be better to use a variable or parameter instead, so that it can be easily updated in the future.

  4. It is not clear if the builder image is publicly available. If it is not, then the pull request should include instructions for how to obtain or build the image.

  5. In the pull-verify-packages-config.yaml file, the run command for generating package images has been simplified, but it is not clear if this will cause any problems or if any important functionality has been lost. It would be good to have more information about why these changes were made.

To address these suggestions, the pull request author could update the description to provide more context, ensure consistent whitespace formatting, use variables or parameters for the builder field, provide instructions for obtaining the builder image if necessary, and document any changes to the run command.

@ti-chi-bot ti-chi-bot bot added size/S and removed size/XS labels Dec 18, 2023
@wuhuizuo
Copy link
Contributor Author

/approve

Copy link

ti-chi-bot bot commented Dec 18, 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 18, 2023
@ti-chi-bot ti-chi-bot bot merged commit 4ba2be3 into main Dec 18, 2023
1 check passed
@ti-chi-bot ti-chi-bot bot deleted the fix/add-ng-monitoring-builder2 branch December 18, 2023 09:40
wuhuizuo added a commit that referenced this pull request Dec 18, 2023
Signed-off-by: wuhuizuo <[email protected]>

---------

Signed-off-by: wuhuizuo <[email protected]>
wuhuizuo added a commit that referenced this pull request Dec 18, 2023
Signed-off-by: wuhuizuo <[email protected]>

---------

Signed-off-by: wuhuizuo <[email protected]>
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