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

fix(apps/prod/tekton/configs/tasks): fix task multi-arch-image-collect for hotfix stories #1348

Merged
merged 3 commits into from
Nov 19, 2024

Conversation

wuhuizuo
Copy link
Collaborator

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

…t for hotfix stories

Signed-off-by: wuhuizuo <wuhuizuo@126.com>
@ti-chi-bot ti-chi-bot bot requested a review from purelind November 19, 2024 11:32
@ti-chi-bot ti-chi-bot bot added area/apps env/prod will deploy on the main product cluster labels Nov 19, 2024
Copy link
Contributor

ti-chi-bot bot commented Nov 19, 2024

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, it seems that the PR is fixing a task called "multi-arch-image-collect" for hotfix stories. The diff shows that the PR adds two new image tags and changes the way tags are parsed and trimmed.

One potential problem is that the changes made to the "multi-arch-image-collect" task may have unintended consequences. It's important to thoroughly test the changes before merging the PR to ensure that the task is working as expected.

As for fixing suggestions, it would be a good idea to create a separate branch to test the changes before merging them into the main branch. Additionally, it may be helpful to add more documentation to the PR description to explain the reasoning behind the changes and how they were tested. This will make it easier for reviewers and future contributors to understand the changes and their impact.

@ti-chi-bot ti-chi-bot bot added the size/S label Nov 19, 2024
Copy link
Contributor

ti-chi-bot bot commented Nov 19, 2024

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, it seems that the changes are meant to fix a task related to multi-arch image collection for hotfix stories. The changes include updating the image version, fixing the tags array, and adding some conditional statements to handle semver tags.

However, there are some potential issues that I noticed in the pull request:

  1. The pull request description is very brief and it does not give enough context about the changes made in the code. It would be helpful to add more details in the description to help the reviewers understand the changes better.

  2. The commit message does not follow the conventional commit format. It is important to follow a consistent commit message format to make it easier to understand the intent of the changes made in the code.

  3. In the tags array, the += operator is used to append elements to the array. This can cause issues if the tags array is not initialized properly. It would be safer to initialize the array first and then append elements to it.

  4. The sed command used in the script to trim the commit SHA suffix part may not work as expected if the commit SHA is less than 7 characters long. It would be better to use a more robust regular expression to handle this case.

To fix these issues, I suggest the following changes:

  1. The pull request description should be updated with more details about the changes made in the code.

  2. The commit message should follow the conventional commit format.

  3. The tags array should be initialized before appending elements to it. For example, tags=("$tag") can be changed to tags=("$tag" "") to initialize the array with two elements.

  4. A more robust regular expression can be used to trim the commit SHA suffix part. For example, sed -E 's/[-][0-9a-f]{7,}$//' can be used instead of sed -E 's/[-][0-9a-f]{7,40}//g'.

Copy link
Contributor

ti-chi-bot bot commented Nov 19, 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, this pull request is intended to fix the multi-arch-image-collect task in the apps/prod/tekton/configs/tasks/ directory for hotfix stories. The main changes are adding two new images, v6.5.7-20241119-4f2073d_linux_amd64 and v8.1.0-alpha-123-g1234567_linux_amd64, and modifying the tags part of the script to handle semver tags differently.

There are a few potential problems with this pull request. First, there is no detailed explanation for the addition of the two new images, which could cause confusion for future maintainers. Second, the script modification for handling semver tags might be unnecessary since the current implementation might already handle them correctly. Finally, there are no tests included with this pull request which makes it difficult to verify that the changes work as intended.

To address these issues, I suggest adding a brief explanation for the addition of the two new images in the PR description or the commit message. Additionally, it might be helpful to provide an example of a semver tag that wasn't being handled correctly before the modification. Finally, I encourage the author to add tests to verify that the changes work as expected.

@wuhuizuo
Copy link
Collaborator Author

/approve

Copy link
Contributor

ti-chi-bot bot commented Nov 19, 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 Nov 19, 2024
@ti-chi-bot ti-chi-bot bot merged commit 94679ba into main Nov 19, 2024
4 checks passed
@ti-chi-bot ti-chi-bot bot deleted the fix/multi-arch-image-collect-task branch November 19, 2024 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved area/apps env/prod will deploy on the main product cluster size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant