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): add tiup-publisher #1280

Merged
merged 1 commit into from
Oct 8, 2024
Merged

Conversation

wuhuizuo
Copy link
Collaborator

@wuhuizuo wuhuizuo commented Oct 8, 2024

This pull request adds a new service, tiup-publisher, to the production environment. The changes include updates to the kustomization files and the addition of HelmRelease configurations for both production and staging mirrors.

Additions to tiup-publisher service:

HelmRelease configurations:

Signed-off-by: wuhuizuo [email protected]

@ti-chi-bot ti-chi-bot bot requested a review from purelind October 8, 2024 14:34
@ti-chi-bot ti-chi-bot bot added area/apps env/prod will deploy on the main product cluster labels Oct 8, 2024
Copy link
Contributor

ti-chi-bot bot commented Oct 8, 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 main changes are:

  • A new resource called tiup-publisher has been added to the prod application.
  • tiup-publisher is a HelmRelease object that has two sub-resources: release-prod-mirror.yaml and release-staging-mirror.yaml.

It seems that the pull request is adding a new feature to the application, which is a good thing. However, there are some potential issues to consider:

  • It is not clear from the pull request description what is the purpose of tiup-publisher and how it is related to the other resources in the application. A more detailed description would be helpful.
  • The version of tiup-publisher is hardcoded in the HelmRelease objects. This could cause problems if the version becomes outdated or incompatible with other components in the application. It would be better to use a variable or a parameter to specify the version.
  • The tiup-credentials secret is mounted as a volume in the tiup-publisher HelmRelease objects. This could pose a security risk, as the secret is not encrypted in transit. It would be better to use a more secure way of passing credentials, such as Kubernetes secrets or environment variables.

To fix these issues, I would suggest the following:

  • Add a more detailed description of tiup-publisher and its purpose in the pull request description.
  • Use a variable or a parameter to specify the version of tiup-publisher in the HelmRelease objects.
  • Use Kubernetes secrets or environment variables to pass credentials to tiup-publisher, instead of mounting the tiup-credentials secret as a volume.

@ti-chi-bot ti-chi-bot bot added the size/L label Oct 8, 2024
@wuhuizuo
Copy link
Collaborator Author

wuhuizuo commented Oct 8, 2024

/approve

Copy link
Contributor

ti-chi-bot bot commented Oct 8, 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 Oct 8, 2024
@ti-chi-bot ti-chi-bot bot merged commit 54c640e into main Oct 8, 2024
4 checks passed
@ti-chi-bot ti-chi-bot bot deleted the feature/add-tiup-publisher branch October 8, 2024 14:41
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/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant