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

CI: fix chart release flow, skip the exist chart version. #820

Merged
merged 4 commits into from
Dec 4, 2023

Conversation

wuhuizuo
Copy link
Collaborator

@wuhuizuo wuhuizuo commented Dec 4, 2023

Signed-off-by: wuhuizuo [email protected]

@ti-chi-bot ti-chi-bot bot requested review from jayl1e and purelind December 4, 2023 07:17
Copy link
Contributor

ti-chi-bot bot commented Dec 4, 2023

I have already done a preliminary review for you, and I hope to help you do a better job.
Based on the pull request's title and description, the key changes are:

  • Fixing the chart release flow
  • Skipping an existing chart version

The pull request's diff shows that a with parameter has been added to the Run chart-releaser step in the workflow file. The skip_existing parameter has been set to true.

Potential problems:
It's difficult to identify any potential problems based on the information given. However, if the chart-releaser's previous behavior was to overwrite existing chart versions, then skipping an existing chart version might not be desirable in some cases.

Fixing suggestions:
If the chart-releaser's previous behavior was to overwrite existing chart versions, the pull request's author should clarify why skipping an existing version is necessary. If the chart-releaser already had the ability to skip existing chart versions, then the pull request is likely fine as-is.

@ti-chi-bot ti-chi-bot bot added the size/XS label Dec 4, 2023
@wuhuizuo
Copy link
Collaborator Author

wuhuizuo commented Dec 4, 2023

/approve

Copy link
Contributor

ti-chi-bot bot commented Dec 4, 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 4, 2023
Copy link
Contributor

ti-chi-bot bot commented Dec 4, 2023

I have already done a preliminary review for you, and I hope to help you do a better job.
Based on the pull request title and description, the key changes in this pull request are:

  • A fix has been made to the chart release flow.
  • The exist chart version is now skipped during the chart release.

As for potential problems, none can be identified based on the provided information.

As for suggestions for fixing, since no problem has been identified, no fix is necessary. However, it is important to ensure that all tests pass and that the code is thoroughly reviewed before merging the pull request.

Copy link
Contributor

ti-chi-bot bot commented Dec 4, 2023

I have already done a preliminary review for you, and I hope to help you do a better job.
Based on the Pull Request, it seems that it adds skip_existing to the chart-releaser-action, which will allow the action to skip already existing chart versions. It also modifies the Chart.yaml and values.yaml files.

There are no potential problems identified in this Pull Request. However, some suggestions for improvement are:

  1. It is always good to provide a detailed description of the changes made in the Pull Request.
  2. The commit message could be more descriptive and informative to help other developers understand the changes without having to read the entire Pull Request description.

Other than that, the changes seem reasonable and straightforward. Therefore, I would suggest merging this Pull Request.

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

ti-chi-bot bot commented Dec 4, 2023

I have already done a preliminary review for you, and I hope to help you do a better job.
This pull request has the following changes:

  • A new workflow file has been added for charts lint testing.
  • Existing workflow file for chart release has been updated to include the skip_existing parameter.
  • Removed ct list-changed and ct lint steps from all charts test workflows.
  • Minor formatting changes in some chart files.

It is a good practice to include more information in the pull request description to explain the motivation behind each change.

Potential problems:

  • It is not clear why ct list-changed and ct lint steps have been removed from all charts test workflows. These steps may be useful to reduce the test time by only running on the changed charts.
  • The skip_existing parameter may cause the existing charts to be skipped during the release process. It is important to ensure that this is the intended behavior.

Fixing suggestions:

  • Add more information to the pull request description to explain the motivation behind removing ct list-changed and ct lint steps from all charts test workflows.
  • Add a comment to the workflow file for chart release to explain the purpose of the skip_existing parameter and ensure that this is the intended behavior.
  • If skip_existing is not intended, remove it from the workflow file for chart release. Otherwise, ensure that the charts that need to be released are not skipped.
  • Check if the formatting changes are necessary and keep them consistent throughout the chart files.

@ti-chi-bot ti-chi-bot bot added size/L and removed size/S labels Dec 4, 2023
Copy link
Contributor

ti-chi-bot bot commented Dec 4, 2023

I have already done a preliminary review for you, and I hope to help you do a better job.
Review for Pull Request #X

The changes in this pull request are aimed at fixing the chart release flow by skipping the existing chart version. The changes made include adding a new file charts_lint-test.yaml with lint and test jobs for the charts. Another change was adding a skip_existing flag in charts_release.yaml file to skip existing chart versions. There were also some minor changes to some chart files.

The changes made in this pull request seem to be minimal and appropriate to fix the problem. However, there are some areas that the author could improve on, such as:

  • The pull request description is not informative enough. The author could add more detail about the changes made and the problem they are trying to solve.
  • It is unclear if the changes made to the chart files are necessary or not. The author could provide more context about why these changes were made.

Overall, the changes look good and are likely to solve the problem. The author could improve the pull request by providing more context and details about the changes made.

Copy link
Contributor

ti-chi-bot bot commented Dec 4, 2023

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

Review for PR: CI: fix chart release flow, skip the exist chart version.

Summary

This pull request adds a new GitHub workflow file for lint testing, modifies the existing workflow file for releasing charts to skip existing versions, and updates the Git CDN chart.

Changes Made

  1. A new GitHub workflow file, charts_lint-test.yaml, has been added for lint testing the charts.
  2. The charts_release.yaml file has been modified to skip chart versions that already exist.
  3. The Git CDN chart has been updated to add a maintainer and comment.

Potential Problems

There are no potential problems with this PR.

Suggestions for Improvement

No suggestions for improvement, as the code looks good.

@ti-chi-bot ti-chi-bot bot merged commit 469ac5d into main Dec 4, 2023
5 checks passed
@ti-chi-bot ti-chi-bot bot deleted the fix/chart-relaese-flow branch December 4, 2023 07:37
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