-
Notifications
You must be signed in to change notification settings - Fork 8
refactor(apps/prod,apps/prod2): move tibuild to prod2 env #1798
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have already done a preliminary review for you, and I hope to help you do a better job.
Summary:
This PR refactors the deployment of the tibuild service by moving it from the prod environment to the prod2 environment. The approach involves removing tibuild from apps/prod/kustomization.yaml and adding it under apps/prod2/tibuild with a versioned directory structure (v1 and v2). The deployment, service, ingress, and configuration files are relocated and slightly updated to reflect the new environment and secret setup. Overall, the changes are organized and mostly consistent, but there are some points to address to improve maintainability and clarity.
Critical Issues
- Potential Secret Name Mismatch (apps/prod2/tibuild/v1/deployment.yaml, lines ~41-50)
- The volume references a secret named
v1-configwhich is created byexternal-secret-config.yaml. While this matches, verify that the secret data (e.g.,config.yaml) matches what the application expects. Secret key names and formats should be consistent with the originaltibuild-configssecret to avoid runtime failures. - Suggestion: Confirm the secret contents and update either the secret or the deployment accordingly.
- The volume references a secret named
Code Improvements
-
Consistency in Volume and VolumeMount Naming
- In
deployment.yamlthe volume and volumeMount are renamed fromconfigstoconfig. This breaks consistency with the original name and might cause confusion or errors if other parts expect the older name. - Suggestion: Maintain the original naming (
configs) unless there is a specific reason to change it. If changed, update all relevant references and document the reason.
volumeMounts: - name: configs # instead of config mountPath: "/app/configs" readOnly: true volumes: - name: configs secret: secretName: v1-config
- In
-
Versioned Directory Structure Clarity
- The introduction of
v1andv2directories underprod2/tibuildis good for versioning but theapps/prod2/tibuild/kustomization.yamlreplaces references tov2/external-secret-config.yamlandv2/release.yamlwith justv1andv2directories, which is ambiguous. - Suggestion: Explicitly list resources in the
kustomization.yamlfiles or ensure the directory structure is clear and documented to avoid confusion.
- The introduction of
-
Image Tag Update Justification
- The image tag was updated from
v2025.11.16-1-g616fce1tov2025.12.14-1-g9dbf1e7. This is presumably intentional, but there is no PR description or comment explaining the reason. - Suggestion: Add a comment or PR description explaining the image update to help reviewers understand the change context.
- The image tag was updated from
Best Practices
-
Missing PR Description and Context
- The PR description is empty, which makes it hard to understand the motivation or the scope fully.
- Suggestion: Add a clear description explaining why
tibuildis moved fromprodtoprod2, what impact this has, and any required follow-up actions.
-
Documentation for New Environment
- Consider adding or updating README or deployment documentation to reflect the addition of
prod2and the new versionedtibuildconfiguration.
- Consider adding or updating README or deployment documentation to reflect the addition of
-
Testing Coverage
- There is no indication of tests or validation steps for this migration.
- Suggestion: Add or update integration tests or deployment validation steps to ensure
tibuildworks correctly inprod2.
-
Naming Conventions
- The secret name
v1-configis generic; consider a more descriptive name liketibuild-v1-configto avoid collisions and improve clarity.
- The secret name
Overall, this PR moves the tibuild component cleanly but would benefit from improved documentation, consistency checks, and verification of secret configurations to ensure a smooth transition.
Summary of ChangesHello @wuhuizuo, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request undertakes a significant refactoring of the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request refactors the tibuild application by moving it from the prod to the prod2 environment and splitting it into v1 and v2 configurations. The overall file structure changes are logical. However, I've found a critical issue in the Kustomize configuration for the v1 application that will prevent it from deploying correctly. The v1 configuration specifies an incorrect namespace and contains a YAML syntax error. My review comment provides details on how to resolve this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have already done a preliminary review for you, and I hope to help you do a better job.
Summary
This PR refactors the deployment of the tibuild app by moving it from the prod environment to the prod2 environment. It achieves this by removing tibuild from the apps/prod/kustomization.yaml and reorganizing all related tibuild manifests under apps/prod2/tibuild/ with versioned subdirectories (v1, v2). The PR also updates image versions and secret references accordingly. Overall, the changes are well-structured and mostly consistent, improving environment separation and maintainability.
Critical Issues
- Secret name inconsistency
- File:
apps/prod2/tibuild/v1/deployment.yaml(lines ~40-50) - Issue: The volume references secret
v1-config, which must exactly match the secret created byexternal-secret-config.yaml. This looks consistent, but confirm that the secret namev1-configis properly provisioned in the cluster. Any mismatch will cause pod startup failures. - Suggestion: Double-check that the secret name in
external-secret-config.yaml(v1-config) matches thesecretNamein the deployment. Consider adding a comment to clarify this linkage.
- File:
Code Improvements
-
Namespace specification consistency
- File:
apps/prod2/tibuild/v1/kustomization.yaml(line 2) - Issue: The
namespacefield is removed compared to the previousapps/prod/tibuild/kustomization.yaml(which hadnamespace: apps). However, the deployment specifiesnamespace: tibuildat a higher level inapps/prod2/tibuild/kustomization.yaml. This might cause confusion or deployment issues if the namespaces are not consistent across all resources. - Suggestion: Explicitly add or document namespace settings in each kustomization or ensure the base
apps/prod2/tibuild/kustomization.yamlsets it correctly. Prefer explicitly specifying the namespace in each kustomization for clarity.
- File:
-
Kustomization resource lists formatting
- Files:
apps/prod/tibuild/kustomization.yaml→apps/prod2/tibuild/v1/kustomization.yaml - Issue: The original used a dash list with indentation for resources, the new version uses inline dash-prefixed resources. Both work, but it is best to be consistent with formatting for readability.
- Suggestion: Align style with the rest of the repo’s kustomization files (typically one item per line with consistent indent).
- Files:
-
Versioning folder structure clarity
- Files:
apps/prod2/tibuild/v1/,apps/prod2/tibuild/v2/ - Issue: The use of both
v1andv2directories for tibuild versions is good for version segregation. However, the rootapps/prod2/tibuild/kustomization.yamlnow references both by- v1and- v2, but thev2folder contents are minimal (onlyexternal-secret-config.yamlandrelease.yaml) and not shown fully. - Suggestion: Ensure that the
v2folder contains all necessary resources (deployment, service, ingress) or document why it differs. Consider adding a README inprod2/tibuildexplaining the versioning strategy.
- Files:
Best Practices
-
PR description improvement
- Issue: The PR description is empty, which misses the opportunity to explain the motivation, testing done, and usage notes.
- Suggestion: Add a brief description to the PR explaining why
tibuildis moved toprod2, any impact on users, and any required post-merge actions.
-
Testing coverage and validation
- Issue: No mention or evidence of testing (e.g., CI pipeline runs, deployment validation, smoke tests).
- Suggestion: Ensure this environment move is covered by integration tests or manual deployment validation. Add notes or tests if possible.
-
Comments and documentation
- Files: All moved files (deployment.yaml, kustomization.yaml, external-secret-config.yaml)
- Issue: No comments explaining purpose of resources or differences between
v1andv2. - Suggestion: Add comments or README clarifying the intent of each version directory and key configuration differences.
Overall, this PR achieves a clean environment migration for tibuild with proper resource relocation and versioning. Addressing namespace clarity, documentation, and testing coverage will improve maintainability and reduce risk during deployment.
|
Fixed the review issues:
Changes are committed and pushed to resolve the kustomization configuration problems mentioned in the review. |
|
The critical kustomization issues have been resolved:\n\n✅ Removed from to prevent namespace conflict\n✅ Fixed YAML indentation in the resources list\n\nThe changes are committed and pushed. This resolves the critical deployment issues mentioned in the review. |
|
/approve |
|
[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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/hold |
|
/unhold |
No description provided.