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(apps/prod/tekton/configs): add task and trigger to publish files to fileserver from oci artifact #1302

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

wuhuizuo
Copy link
Collaborator

Signed-off-by: wuhuizuo [email protected]

… to fileserver from oci artifact

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

ti-chi-bot bot commented Oct 29, 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 Oct 29, 2024
Copy link
Contributor

ti-chi-bot bot commented Oct 29, 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, the following key changes have been made:

  • A new task called publish-fileserver-from-oci-artifact.yaml has been added to publish files to a fileserver from an OCI artifact.
  • A new trigger called push-oci-artifact-to-fileserver.yaml has been added to push an OCI artifact to a fileserver.
  • The kustomization.yaml file has been modified to add publish-fileserver-from-oci-artifact.yaml and push-oci-artifact-to-fileserver.yaml files in the resources section.
  • A new trigger for artifact-push-on-harbor-with-trunk-branch-tags has been added.

There are no apparent potential problems in this pull request. However, here are some suggestions for fixing:

  • It is recommended to add more details to the pull request description, such as why these changes are made and how they will affect the CI/CD pipeline.
  • It is always better to add some comments in the code explaining the purpose of the code, which makes it easier for others to understand.
  • It is better to avoid hard-coding the URLs in the code and use environment variables instead.

Overall, the changes seem to be fine, and it can be merged after addressing the suggestions.

@ti-chi-bot ti-chi-bot bot added the size/L label Oct 29, 2024
Copy link
Contributor

ti-chi-bot bot commented Oct 29, 2024

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

The pull request adds a new task and trigger to publish files to a file server from an OCI artifact. The changes include adding a new task definition file, a new trigger template file, and updating the kustomization.yaml files to include these new files.

One potential problem is that the default value for publisher-url in the task definition is set to the staging publisher mirror. This may not be appropriate for production use. It would be better to use a parameter or environment variable to set the publisher mirror URL based on the deployment environment.

Another potential issue is that the task script uses a hardcoded path to the publisher-cli binary. This may not work if the binary is installed in a different location or if the task is run in a different operating system. It would be better to use a parameter or environment variable to set the path to the publisher-cli binary.

To fix these issues, the following changes can be made:

  • Add a parameter or environment variable to set the publisher mirror URL based on the deployment environment.

  • Use a parameter or environment variable to set the path to the publisher-cli binary in the task script.

Once these changes are made, the pull request can be approved and merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/apps env/prod will deploy on the main product cluster size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant