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/triggers): adjust the trigger to just publish artifacts on trunk branch and tags to fileserver #1371

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

wuhuizuo
Copy link
Collaborator

Signed-off-by: wuhuizuo [email protected]

…blish artifacts on trunk branch and tags to fileserver

Signed-off-by: wuhuizuo <[email protected]>
@ti-chi-bot ti-chi-bot bot requested a review from purelind December 10, 2024 03:04
@ti-chi-bot ti-chi-bot bot added the area/apps label Dec 10, 2024
Copy link
Contributor

ti-chi-bot bot commented Dec 10, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from wuhuizuo, ensuring that each of them provides their approval before proceeding. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found 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 env/prod will deploy on the main product cluster label Dec 10, 2024
Copy link
Contributor

ti-chi-bot bot commented Dec 10, 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, the key changes in this pull request are to adjust the trigger to restrict the publishing of artifacts to trunk branch and tags to the fileserver only. The changes are made in the artifact-push-on-harbor.yaml file, where the regular expression pattern for matching the tag is updated.

However, there are a few potential problems that need to be addressed in this pull request:

  1. The regular expression pattern "^(master|main|v[0-9]+[.][0-9]+[.][0-9]+)_linux_amd64$" only matches the tags that start with "master", "main", or "v" followed by three digits separated by dots and "linux_amd64". It may miss matching tags that do not follow this pattern. Thus, it may not be able to restrict the publishing of all tags to the fileserver only.

  2. The pull request lacks a detailed description of the problem or use-case that prompted the change. It would be helpful to have more context regarding why these changes are necessary.

To address these issues, you can suggest the following changes:

  1. Update the regular expression pattern to "^(master|main|v[0-9]+[.][0-9]+[.][0-9]+).+_linux_amd64$" to match all tags that start with "master", "main", or "v" followed by three digits separated by dots and end with "linux_amd64". This will ensure that the trigger only publishes artifacts with matching tags to the fileserver.

  2. Ask the pull request author to provide more context regarding the problem or use-case that led to these changes. This will help in understanding the rationale behind the changes and ensure that they are addressing the right problem.

@ti-chi-bot ti-chi-bot bot added the size/XS label Dec 10, 2024
Copy link
Collaborator Author

@wuhuizuo wuhuizuo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/hold

we must modify the config secrets manually before merge it.

@wuhuizuo wuhuizuo force-pushed the main branch 2 times, most recently from b6f7fe9 to 10a5805 Compare January 17, 2025 06:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/apps do-not-merge/hold env/prod will deploy on the main product cluster size/XS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant