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(scripts/artifacts): add shell function publish_tiup_from_oci_artifact_repos #2955

Merged
merged 1 commit into from
Jun 4, 2024

Conversation

wuhuizuo
Copy link
Collaborator

@wuhuizuo wuhuizuo commented May 8, 2024

Signed-off-by: wuhuizuo wuhuizuo@126.com

…tifact_repos`

Signed-off-by: wuhuizuo <wuhuizuo@126.com>
@ti-chi-bot ti-chi-bot bot requested a review from purelind May 8, 2024 09:22
Copy link

ti-chi-bot bot commented May 8, 2024

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

Based on the PR title and description, it seems that this PR adds a new shell function called publish_tiup_from_oci_artifact_repos() to the existing tag-rc2ga-on-repos.sh script. This new function is responsible for publishing TiUP packages from OCI artifact repositories.

There are no immediate problems with this PR, but here are some suggestions for improvement:

  1. The PR description should provide more context about the changes. What problem does this PR solve? Why is this change necessary? It would be helpful to provide some background information to make it easier for reviewers to understand the changes.

  2. The function name publish_tiup_from_oci_artifact_repos() is quite long and may be difficult to read. Consider shortening the name to something like publish_tiup_from_oci().

  3. The publish_tiup_oci_repo() function is called from within a loop in publish_tiup_from_oci_artifact_repos(). It might be more efficient to pass an array of repos and tags to publish_tiup_from_oci_artifact_repos() and then loop through them once inside the function.

  4. The tkn command in publish_tiup_oci_repo() is not defined in the script. It's assumed that this command is available on the system where the script will be run. It would be helpful to provide some guidance on how to install this command or any other dependencies required by the script.

  5. The publish_tiup_oci_repo() function assumes that the TiUP packages are available in specific repositories and tags. It might be better to make these values configurable through command-line arguments or environment variables.

Overall, the changes look good and seem to serve a useful purpose. It would be helpful to address the suggestions above to make the script more robust and maintainable.

@ti-chi-bot ti-chi-bot bot added the size/M label May 8, 2024
@wuhuizuo wuhuizuo added the lgtm label Jun 4, 2024
@wuhuizuo
Copy link
Collaborator Author

wuhuizuo commented Jun 4, 2024

/approve

Copy link

ti-chi-bot bot commented Jun 4, 2024

[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 Jun 4, 2024
@ti-chi-bot ti-chi-bot bot merged commit 47aeb26 into main Jun 4, 2024
1 check passed
@ti-chi-bot ti-chi-bot bot deleted the feature/add-republish-tiup-shell-func branch June 4, 2024 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

1 participant