feat: Implement automated database migration flow for PostgREST services#79
Merged
NgocAnhDo26 merged 57 commits intoMay 31, 2026
Conversation
…sers/hph/impl/template-dotnet
…ess and Create Spring Boot (Java) Backstage Template (#66) Co-authored-by: Do Tan Ngoc Anh <119093159+NgocAnhDo26@users.noreply.github.com>
…ADME and schema files
…for Kubernetes secret management
- Added `triggerType: db-migrate` to HeliosApp configuration for better migration handling. - Introduced a new Kubernetes namespace manifest for better resource management. - Updated PostgREST configuration to ensure log levels are consistently quoted. - Modified webhook creation to specify a dedicated listener for DB migration triggers. - Reorganized Tekton pipeline definitions to include a dedicated DB migration pipeline. - Updated Git clone task to transform localhost URLs for in-cluster access. - Enhanced database migration task to use Kubernetes secrets for database credentials. - Improved PostgREST reload task to utilize dynamic secret names for better flexibility. - Refined event listener to trigger DB migration pipelines based on specific directory changes. - Added new trigger template for DB migration to streamline pipeline execution on relevant events.
…ate-dotnet Setup .NET template
…to-deploy-fixed' into features/sprint4-backend-templates-db-injection
…-fix-templates Fixes and improvements for .NET, Spring Boot and Postgrest templates
…sers/ndvh/impl/hasura-graphql-template
…ra-graphql-template setup hasura-graphql-template
- Updated PostgREST template to remove repoName and enhance naming consistency. - Added ArgoCD PreSync Job template for executing database migrations before deployment. - Created Tekton pipeline for building migration Docker images with golang-migrate. - Introduced patterns for building migration images and triggering migration pipelines. - Enhanced documentation to outline the migration process and testing guide. - Added troubleshooting steps for common migration issues and verification checks.
📝 WalkthroughWalkthroughPR này thêm luồng migration PostgREST: operator phát hiện trait database và tạo PreSync RBAC/ServiceAccount + annotation Job; Tekton build/push image migration; ArgoCD chạy PreSync job trước sync và PostSync restart pods; kèm CRD/kustomize, CUE, scaffolder, Dockerfile và docs. ChangesPostgREST Database Migration Flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
3 tasks
Base automatically changed from
features/sprint4-backend-templates-db-injection
to
main
May 26, 2026 15:13
…ith-ArgoCD-presync-migrations
Collaborator
Author
✅ Database Migration Auto-Generation - All Criteria MetCriterion 1: Migrations Run Automatically Before Deployment ✅
Criterion 2: Failed Migrations Block ArgoCD Sync ✅
Criterion 3: PostgREST Reflects Schema Changes Immediately ✅
SummaryAll 3 acceptance criteria have been successfully validated through end-to-end testing: ✅ Operator auto-generates PreSync Job resources when HeliosApp has database trait Status: Ready for production deployment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 25
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/operator/config/manager/kustomization.yaml`:
- Around line 5-8: Replace the default "latest" image tag with a pinned tag and
require/pass IMG from CI: update the images entry in kustomization.yaml to use a
specific version (not "latest") for name: controller/newName: controller/newTag:
<pin>, and change the Makefile's IMG ?= controller:latest to either fail if IMG
is unset or remove the default (e.g., require IMG be provided by CI) so that the
deploy path using kustomize edit set image controller=${IMG} always receives an
explicit, reproducible tag; ensure any documentation/CI pipelines are updated to
supply the chosen IMG variable.
In `@apps/operator/internal/controller/argocd/application.go`:
- Around line 50-71: The code redundantly recreates the syncPolicy map for
HasDatabaseTrait(heliosApp) then reassigns it; instead locate the existing
spec["syncPolicy"] (map[string]any) and simply append
"SkipDryRunOnMissingResource=true" to its "syncOptions" slice without rebuilding
the whole map — update the block around HasDatabaseTrait/heliosApp to fetch
spec["syncPolicy"].(map[string]any), append the option to
syncPolicy["syncOptions"].([]any), and leave spec["syncPolicy"] otherwise
unchanged.
In `@apps/operator/internal/controller/argocd/presync.go`:
- Around line 42-51: The duplicate database-trait detection in
ReconcilePreSyncResources should be replaced by calling the existing
HasDatabaseTrait helper instead of re-implementing the loop; remove the local
for-loops that set hasDatabaseTrait and replace with a single call like
hasDatabaseTrait := HasDatabaseTrait(heliosApp) (or use whatever signature
HasDatabaseTrait provides) so ReconcilePreSyncResources and the existing
HasDatabaseTrait (lines ~278-288) share the same logic.
- Around line 225-232: The secret name used for PGRST_DB_URI currently hardcodes
fmt.Sprintf("%s-db-credentials") but the CRD/docs and heliosapp_types.go expect
either heliosApp.Spec.DatabaseSecretRef (when provided) or the convention
"{appName}-db-secret"; update the env var creation (the block that sets
"PGRST_DB_URI") to resolve the secret name by first checking
heliosApp.Spec.DatabaseSecretRef (use its Name) and falling back to
fmt.Sprintf("%s-db-secret", heliosApp.Name) before constructing the secretKeyRef
so the code follows the documented convention.
- Around line 206-255: The Job manifest incorrectly places serviceAccountName at
the Job spec root (symbol: "serviceAccountName") instead of inside the pod
template spec; move serviceAccountName into the nested "template" -> "spec" map
so the pod uses the created ServiceAccount (e.g., the fmt.Sprintf("%s-migrator",
heliosApp.Name) value). Also fix the container securityContext by renaming the
invalid "fsReadOnlyRootFilesystem" to the correct container-level field
"readOnlyRootFilesystem" inside the container's "securityContext" (symbol:
"securityContext") and ensure it remains under the container spec, not the
pod-level spec.
In `@apps/portal/examples/postgrest-template/content/source/Dockerfile.migrate`:
- Around line 13-28: The image runs as root; create and use a non-root user for
the PreSync job: add a non-root user (e.g., "migrator"), ensure /migrations,
/usr/local/bin/migrate and /entrypoint.sh are owned and executable by that user
(use chown/chmod during image build), set USER migrator (and optionally HOME)
before the ENTRYPOINT, and ensure the wrapper script /entrypoint.sh does not
require root-only operations; this keeps ENTRYPOINT ["/entrypoint.sh"] but runs
it as the non-root "migrator" user.
- Line 10: The Dockerfile currently pipes the remote archive
(migrate-linux-amd64.tar.gz) straight into tar which skips integrity checks;
change the RUN step to fetch the release artifact and its checksum/signature
(e.g., the project's SHA256 or GPG signature), verify the download against that
checksum/signature, and only then extract to /tmp/ and install—fail the build on
mismatch. Concretely, update the RUN that references migrate-linux-amd64.tar.gz
to first download the checksum/signature for the same v4.17.0 release, perform
verification (sha256sum -c or gpg --verify), then extract the archive and remove
downloaded files if verification succeeds.
In `@apps/portal/examples/postgrest-template/template.yaml`:
- Line 131: Don't use parameters.name as webhookSecret because it's predictable;
create and reference a dedicated secret instead (e.g., add a new template
parameter named webhookSecret of type securestring or fetch from a secret
store/KeyVault) and replace all uses of parameters.name for webhookSecret (the
occurrences around webhookSecret and lines mentioned) with that secure parameter
or a KeyVault reference so the webhookSecret is unpredictable and not exposed in
the template.
In `@cue/definitions/argocd/pre-sync-job.cue`:
- Around line 72-76: The container securityContext uses an incorrect field name
fsReadOnlyRootFilesystem; replace it with the correct Kubernetes field name
readOnlyRootFilesystem in the same securityContext block (refer to the
securityContext object around the pre-sync job definition) so the container spec
uses readOnlyRootFilesystem: true while keeping runAsNonRoot, runAsUser, and
other settings unchanged.
- Around line 144-146: The container image for the kubectl sidecar is using the
unstable "bitnami/kubectl:latest" tag; change the image in the containers block
for the entry with name "kubectl" to a pinned tag or digest (e.g.,
bitnami/kubectl:<semver> or bitnami/kubectl@sha256:...) to ensure reproducible
deployments, and optionally set imagePullPolicy to IfNotPresent; update the
image string where containers: [{ name: "kubectl" image: ... }] is declared.
- Around line 36-39: serviceAccountName is placed directly under the Job spec
but must be under spec.template.spec like in presync.go; move the
serviceAccountName entry out of the top-level spec block and add it inside
spec.template.spec (ensuring spec.template exists), keeping backoffLimit and
ttlSecondsAfterFinished at the top spec; mirror the structure used in presync.go
so the Job manifest places serviceAccountName under spec.template.spec.
- Around line 24-34: The ArgoCD hook keys are duplicated in both labels and
annotations; remove the redundant entries from the labels block so only
annotations contain "argocd.argoproj.io/hook" and
"argocd.argoproj.io/hook-deletion-policy". Keep the existing labels like "app":
appName and "job-type": "db-migration" but delete the two ArgoCD keys from the
labels map (refer to the labels and annotations blocks in the pre-sync job
definition).
- Around line 107-171: The PostSync job (`#DatabaseMigrationPostSyncJob`) is
missing the Pod securityContext; keep the serviceAccountName under
spec.template.spec (already correct) and add the same securityContext used by
the PreSyncJob into spec.template.spec so the pod-level security settings (e.g.
runAsNonRoot/runAsUser/runAsGroup/fsGroup or whatever the PreSyncJob uses) are
applied consistently; locate `#DatabaseMigrationPostSyncJob` and copy the
securityContext block from the PreSyncJob into its spec.template.spec directly.
In `@cue/definitions/tekton/pipelines/patterns.cue`:
- Around line 199-200: The pipeline currently hardcodes the image tag to
"-migrate:latest" and sets contextSubpath to ".", which makes builds
non-deterministic and breaks repos using a subpath; change the image value
generation to use a deterministic tag parameter (e.g., use
params.\(`#PipelineParams.imageTag.name`) or
params.\(`#PipelineParams.imageRepo.name`)-$(params.<imageTagParam>) instead of
":latest") and replace the hardcoded contextSubpath "." with a parameter
reference (e.g., params.\(`#PipelineParams.contextSubpath.name`) or a dedicated
sourceSubpath param) so both the image tag and build context come from pipeline
params; update references around tekton.#CommonParams.image.name.name and
tekton.#CommonParams.image.contextSubpath.name to use those param symbols.
In `@cue/definitions/tekton/triggers/db-migrate-trigger.cue`:
- Line 72: Dòng đang hardcode trường {name: "db-secret-name", value:
"\(_bp.appName)-db-secret"} làm bỏ qua cấu hình custom; thay bằng lấy giá trị từ
cấu hình secret reference (ví dụ sử dụng databaseSecretRef hoặc trường tương ứng
trong _bp/values) và chỉ fallback về "\(_bp.appName)-db-secret" khi cấu hình đó
không có; cập nhật chỗ chứa chuỗi/field "db-secret-name" để read từ
databaseSecretRef (hoặc tên cấu hình bạn đang dùng) thay vì giá trị cứng.
In `@docs/OPERATOR.md`:
- Line 191: Add a blank line above the Markdown headings that are missing
spacing to satisfy markdownlint: insert an empty line before "#### Tekton
Pipeline: `db-migrate-image`" and likewise before the other affected headings
(the headings at the text matching lines around "199-199" and "205-205" in the
same document) so each heading is separated from the previous paragraph by a
single blank line.
- Around line 282-285: Cập nhật mô tả trong docs/OPERATOR.md để sửa nguồn gốc
của các resource: thay đoạn "When the `database` trait is present, the
scaffolder automatically includes" bằng một câu rõ ràng nói rằng PreSync Job,
ServiceAccount và Kustomization được tạo tự động bởi operator khi reconcile một
HeliosApp có `database` trait (không phải do scaffolder); tham chiếu rõ các tên
phần tử là PreSync Job, ServiceAccount và các RBAC resources để tránh nhầm lẫn.
- Around line 214-221: Update the text in docs/OPERATOR.md to remove the claim
that the scaffolder creates "presync-job.yaml" and instead state that the
scaffolder only generates "namespace.yaml" and "helios-app.yaml" (and other
minimal base files), while the operator automatically creates the PreSync Job
during reconciliation; ensure all mentions of "presync-job.yaml" are replaced or
clarified and reference the scaffolder and operator responsibilities
(scaffolder, namespace.yaml, helios-app.yaml, PreSync Job) so the document is
consistent with other sections.
- Line 203: Chỉnh sửa mục "Location" trong OPERATOR.md để đường dẫn khớp với nội
dung được mô tả: nếu đang nói tới thư mục chứa helios-app.yaml và namespace.yaml
thì giữ Location trỏ tới thư mục gitops; nếu đang nói tới Dockerfile.migrate thì
thay Location thành đường dẫn tới thư mục source/Dockerfile.migrate; tìm và sửa
dòng "Location" trong OPERATOR.md tương ứng với nội dung (ghi rõ gitops khi đề
cập các manifest, hoặc source/Dockerfile.migrate khi đề cập file
Dockerfile.migrate).
- Around line 199-203: Cập nhật phần "GitOps Manifests (created via Template
Scaffolder)" trong docs/OPERATOR.md: thay thế khẳng định rằng presync-job.yaml
và kustomization.yaml được scaffolded vào
apps/portal/examples/postgrest-template/content/gitops/ bằng mô tả chính xác
rằng scaffolder chỉ fetch ./content/gitops và hiện chỉ chứa helios-app.yaml và
namespace.yaml; sửa vị trí của Dockerfile.migrate thành
apps/portal/examples/postgrest-template/content/source/; và ghi rõ PreSync Job
cùng RBAC không nằm trong scaffolded gitops content mà được operator
sinh/managed tại runtime (tham chiếu tới
apps/operator/internal/controller/argocd/presync.go và
cue/definitions/argocd/pre-sync-job.cue) để tránh hiểu lầm.
- Line 257: Docs and operator disagree on the DB secret name/key: the PreSync
job currently looks for "<app>-db-credentials" with key "uri" while the database
trait creates "<component>-db-secret" using key "PGRST_DB_URI" and exposes a
databaseSecretRef default. Update the PreSync implementation to read the
configured databaseSecretRef (use the same naming produced by
GetDatabaseSecretName) and the PGRST_DB_URI key (not "uri"), i.e. modify the
PreSync job templates (pre-sync-job.cue) and presync logic (presync.go) to
respect databaseSecretRef/default and use "PGRST_DB_URI"; alternatively, if you
prefer changing the trait, make GetDatabaseSecretName produce the
"<app>-db-credentials" name and write the connection string under key "uri", and
then update docs to match—choose one approach and make docs, PreSync
(presync.go, pre-sync-job.cue), and database secret generation
(GetDatabaseSecretName / resources.go) consistent.
In `@docs/POSTGREST_MIGRATION_TEST_GUIDE.md`:
- Around line 287-295: The "Example successful output" block currently shows
Flyway logs; update that block to reflect golang-migrate output instead. Replace
the Flyway lines (the fenced code block under "Example successful output") with
the golang-migrate lines: "Running database migrations...", "1/u init
(42.123ms)", and "Migrations completed successfully" so the expected output
matches the project's migration flow.
- Around line 100-111: Update the RBAC verification examples to match the
operator's naming convention: use the HeliosApp name prefix when grepping for
ServiceAccount, ClusterRole and ClusterRoleBinding (they are created with
fmt.Sprintf("%s-migrator", heliosApp.Name), fmt.Sprintf("%s-presync-job-role",
heliosApp.Name) and the similarly prefixed binding in
apps/operator/internal/controller/argocd/presync.go); replace the generic
"migrator", "presync-job-role" and "presync-job-binding" grep patterns with
"<HeliosAppName>-migrator", "<HeliosAppName>-presync-job-role" and the
corresponding "<HeliosAppName>-presync-job-binding" patterns so examples reflect
actual resource names.
- Around line 113-121: The docs hardcode the service namespace (-n el-services)
in Step 2.3 which conflicts with CUE's bundleParams.namespace used when creating
the Tekton EventListener (see template `#TektonEventListener` and
bundleParams.namespace in cue/definitions/tekton/triggers/*.cue); update the
example in docs/POSTGREST_MIGRATION_TEST_GUIDE.md so both kubectl commands use
the same namespace placeholder (-n <namespace>) or add a short note explaining
why this test uniquely requires el-services and that bundleParams.namespace is
otherwise used. Ensure references to the EventListener lookup and the svc lookup
are consistent with bundleParams.namespace.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5a387ceb-e757-4440-a915-095b86d5647c
📒 Files selected for processing (13)
apps/operator/config/crd/bases/app.helios.io_heliosapps.yamlapps/operator/config/manager/kustomization.yamlapps/operator/internal/controller/argocd/application.goapps/operator/internal/controller/argocd/presync.goapps/operator/internal/controller/argocd/reconciler.goapps/portal/examples/postgrest-template/content/source/Dockerfile.migrateapps/portal/examples/postgrest-template/template.yamlcue/definitions/argocd/pre-sync-job.cuecue/definitions/tekton/pipelines/db-migrate-image.cuecue/definitions/tekton/pipelines/patterns.cuecue/definitions/tekton/triggers/db-migrate-trigger.cuedocs/OPERATOR.mddocs/POSTGREST_MIGRATION_TEST_GUIDE.md
Comment on lines
5
to
+8
| images: | ||
| - name: controller | ||
| newName: controller | ||
| newTag: latest |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Kiểm tra xem có overlay production riêng với pinned version không
fd -t f "kustomization" apps/operator/config/ --exec cat {} \; -exec echo "--- File: {}" \;Repository: helios-platform-team/helios-platform
Length of output: 422
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Tìm mọi chỗ dùng newTag: latest trong apps/operator/config
echo "== newTag: latest occurrences =="
rg -n --hidden --no-ignore -S "newTag:\s*latest" apps/operator/config || true
# 2) Liệt kê toàn bộ kustomization.yaml trong apps/operator/config để xem cấu trúc overlay
echo
echo "== kustomization.yaml files =="
fd -t f "kustomization.yaml" apps/operator/config -x echo
# 3) In nội dung kustomization.yaml của thư mục manager (nơi có snippet) để đối chiếu
echo
echo "== manager/kustomization.yaml =="
if [ -f "apps/operator/config/manager/kustomization.yaml" ]; then
nl -ba apps/operator/config/manager/kustomization.yaml | sed -n '1,120p'
else
echo "File not found: apps/operator/config/manager/kustomization.yaml"
fiRepository: helios-platform-team/helios-platform
Length of output: 747
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== manager/kustomization.yaml (with line numbers) =="
cat -n apps/operator/config/manager/kustomization.yaml | sed -n '1,200p'
echo
echo "== Search for prod/production overlays references =="
rg -n --hidden --no-ignore -S "(prod|production|staging|dev|development)" apps/operator/config || true
echo
echo "== Search where manager is included as resource/base =="
rg -n --hidden --no-ignore -S "manager" apps/operator/config/*/kustomization.yaml || trueRepository: helios-platform-team/helios-platform
Length of output: 6353
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== List kustomization.yaml under apps/operator/config (already known) =="
fd -t f "kustomization.yaml" apps/operator/config -x echo
echo
echo "== Show apps/operator/config/default/kustomization.yaml =="
cat -n apps/operator/config/default/kustomization.yaml | sed -n '1,240p'
echo
echo "== Search where apps/operator/config/default (or ../manager) is referenced outside config =="
rg -n --hidden --no-ignore -S "apps/operator/config/default|../manager|operator/config/default|operator/config/manager" apps || true
echo
echo "== Search for \"newTag: latest\" across repo =="
rg -n --hidden --no-ignore -S "newTag:\s*latest" . || trueRepository: helios-platform-team/helios-platform
Length of output: 11237
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== apps/operator/Makefile: tìm các đoạn set image controller=${IMG} =="
rg -n "set image controller=" apps/operator/Makefile
echo
echo "== Hiển thị quanh các dòng 150-240 =="
sed -n '140,240p' apps/operator/Makefile
echo
echo "== Hiển thị quanh các dòng 280-360 =="
sed -n '280,360p' apps/operator/MakefileRepository: helios-platform-team/helios-platform
Length of output: 121
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== apps/operator/Makefile: tìm các đoạn set image controller=... =="
rg -n 'set image controller=' apps/operator/Makefile
echo
echo "== Hiển thị quanh các dòng 140-240 =="
sed -n '140,240p' apps/operator/Makefile
echo
echo "== Hiển thị quanh các dòng 280-360 =="
sed -n '280,360p' apps/operator/MakefileRepository: helios-platform-team/helios-platform
Length of output: 7837
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "^\s*IMG\s*(\?|:|\+=|=)" apps/operator/Makefile || true
echo
echo "== Show lines around any IMG definition =="
# print around likely IMG assignments
rg -n "IMG" apps/operator/Makefile | head -n 50
echo
echo "== Search for \"make deploy\" docs/usage mentioning IMG override =="
rg -n "make (deploy|install)|IMG=" apps/operator/README.md apps/operator/Makefile || trueRepository: helios-platform-team/helios-platform
Length of output: 3046
Pin tag cho image controller thay vì dùng latest
Trong apps/operator/config/manager/kustomization.yaml đang set:
images:
- name: controller
newName: controller
newTag: latestNgoài ra apps/operator/Makefile có IMG ?= controller:latest và make deploy/install chạy kustomize edit set image controller=${IMG}, nên nếu không truyền IMG thì bản deploy sẽ luôn dùng latest, làm giảm reproducibility/khó rollback-debug ở production. Nên pin version cụ thể (hoặc bắt buộc truyền IMG từ CI) thay vì mặc định latest.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/operator/config/manager/kustomization.yaml` around lines 5 - 8, Replace
the default "latest" image tag with a pinned tag and require/pass IMG from CI:
update the images entry in kustomization.yaml to use a specific version (not
"latest") for name: controller/newName: controller/newTag: <pin>, and change the
Makefile's IMG ?= controller:latest to either fail if IMG is unset or remove the
default (e.g., require IMG be provided by CI) so that the deploy path using
kustomize edit set image controller=${IMG} always receives an explicit,
reproducible tag; ensure any documentation/CI pipelines are updated to supply
the chosen IMG variable.
Comment on lines
+50
to
+71
| // Add PreSync hook if database trait exists | ||
| if HasDatabaseTrait(heliosApp) { | ||
| spec["syncPolicy"] = map[string]any{ | ||
| "automated": map[string]any{ | ||
| "prune": true, | ||
| "selfHeal": true, | ||
| }, | ||
| "syncOptions": []any{ | ||
| "CreateNamespace=true", | ||
| }, | ||
| } | ||
|
|
||
| // Add PreSync hook to application | ||
| // Note: PreSync Job is created and managed by PreSyncReconciler | ||
| // This is referenced via Job annotations, not stored in Application spec | ||
| syncPolicy := spec["syncPolicy"].(map[string]any) | ||
| syncPolicy["syncOptions"] = append( | ||
| syncPolicy["syncOptions"].([]any), | ||
| "SkipDryRunOnMissingResource=true", | ||
| ) | ||
| spec["syncPolicy"] = syncPolicy | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Code thêm SkipDryRunOnMissingResource có phần dư thừa.
Lines 52-60 gán lại syncPolicy map giống hệt với lines 30-38 đã định nghĩa trước đó. Chỉ cần append thêm option vào syncOptions hiện có.
♻️ Đề xuất đơn giản hóa
// Add PreSync hook if database trait exists
if HasDatabaseTrait(heliosApp) {
- spec["syncPolicy"] = map[string]any{
- "automated": map[string]any{
- "prune": true,
- "selfHeal": true,
- },
- "syncOptions": []any{
- "CreateNamespace=true",
- },
- }
-
- // Add PreSync hook to application
- // Note: PreSync Job is created and managed by PreSyncReconciler
- // This is referenced via Job annotations, not stored in Application spec
- syncPolicy := spec["syncPolicy"].(map[string]any)
- syncPolicy["syncOptions"] = append(
- syncPolicy["syncOptions"].([]any),
+ // Add SkipDryRunOnMissingResource for PreSync Job support
+ syncPolicy := spec["syncPolicy"].(map[string]any)
+ syncPolicy["syncOptions"] = append(
+ syncPolicy["syncOptions"].([]any),
"SkipDryRunOnMissingResource=true",
)
- spec["syncPolicy"] = syncPolicy
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/operator/internal/controller/argocd/application.go` around lines 50 -
71, The code redundantly recreates the syncPolicy map for
HasDatabaseTrait(heliosApp) then reassigns it; instead locate the existing
spec["syncPolicy"] (map[string]any) and simply append
"SkipDryRunOnMissingResource=true" to its "syncOptions" slice without rebuilding
the whole map — update the block around HasDatabaseTrait/heliosApp to fetch
spec["syncPolicy"].(map[string]any), append the option to
syncPolicy["syncOptions"].([]any), and leave spec["syncPolicy"] otherwise
unchanged.
Comment on lines
+42
to
+51
| // Check if any component has database trait | ||
| hasDatabaseTrait := false | ||
| for _, comp := range heliosApp.Spec.Components { | ||
| for _, trait := range comp.Traits { | ||
| if trait.Type == "database" { | ||
| hasDatabaseTrait = true | ||
| break | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Logic kiểm tra database trait bị duplicate.
ReconcilePreSyncResources có logic kiểm tra trait database (lines 42-51) và HasDatabaseTrait function (lines 278-288) có cùng logic. Nên reuse HasDatabaseTrait trong ReconcilePreSyncResources.
♻️ Đề xuất refactor
func (r *PreSyncReconciler) ReconcilePreSyncResources(
ctx context.Context,
heliosApp *appv1alpha1.HeliosApp,
) error {
log := logf.FromContext(ctx)
- // Check if any component has database trait
- hasDatabaseTrait := false
- for _, comp := range heliosApp.Spec.Components {
- for _, trait := range comp.Traits {
- if trait.Type == "database" {
- hasDatabaseTrait = true
- break
- }
- }
- }
-
- if !hasDatabaseTrait {
+ if !HasDatabaseTrait(heliosApp) {
log.Info("No database trait found, skipping PreSync resource creation")
return nil
}Also applies to: 278-288
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/operator/internal/controller/argocd/presync.go` around lines 42 - 51,
The duplicate database-trait detection in ReconcilePreSyncResources should be
replaced by calling the existing HasDatabaseTrait helper instead of
re-implementing the loop; remove the local for-loops that set hasDatabaseTrait
and replace with a single call like hasDatabaseTrait :=
HasDatabaseTrait(heliosApp) (or use whatever signature HasDatabaseTrait
provides) so ReconcilePreSyncResources and the existing HasDatabaseTrait (lines
~278-288) share the same logic.
Comment on lines
+112
to
+149
| // reconcileRole creates a ClusterRole for Job and Pod management. | ||
| func (r *PreSyncReconciler) reconcileRole(ctx context.Context, heliosApp *appv1alpha1.HeliosApp) error { | ||
| role := &rbacv1.ClusterRole{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: fmt.Sprintf("%s-presync-job-role", heliosApp.Name), | ||
| Labels: map[string]string{ | ||
| "app.kubernetes.io/name": heliosApp.Name, | ||
| "app.kubernetes.io/managed-by": "helios-operator", | ||
| }, | ||
| }, | ||
| Rules: []rbacv1.PolicyRule{ | ||
| { | ||
| APIGroups: []string{"batch"}, | ||
| Resources: []string{"jobs"}, | ||
| Verbs: []string{"get", "list", "watch", "create", "delete"}, | ||
| }, | ||
| { | ||
| APIGroups: []string{""}, | ||
| Resources: []string{"pods"}, | ||
| Verbs: []string{"get", "list", "watch"}, | ||
| }, | ||
| { | ||
| APIGroups: []string{""}, | ||
| Resources: []string{"pods/log"}, | ||
| Verbs: []string{"get"}, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| if err := r.Create(ctx, role); err != nil { | ||
| if apierrors.IsAlreadyExists(err) { | ||
| return nil | ||
| } | ||
| return err | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Kiểm tra xem có finalizer nào được implement cho HeliosApp không
rg -n "finalizer|Finalizer" apps/operator/internal/controller/Repository: helios-platform-team/helios-platform
Length of output: 228
ClusterRole/ClusterRoleBinding cho presync thiếu owner reference và không có finalizer để dọn dẹp khi HeliosApp bị xóa
apps/operator/internal/controller/argocd/presync.go (reconcileRole) tạo rbacv1.ClusterRole nhưng không thiết lập OwnerReference/SetControllerReference, nên không được garbage-collect khi HeliosApp bị xóa. Ngoài ra, trong apps/operator/internal/controller chỉ thấy annotation RBAC cho heliosapps/finalizers trong heliosapp_controller.go, không thấy implement finalizer/cleanup để xóa các RBAC cluster-scoped tương ứng.
Cân nhắc:
- Dùng
Role/RoleBinding(namespace-scoped) nếu chỉ cần quyền trong namespace của app - Hoặc implement finalizer trên
HeliosAppđể cleanupClusterRole/ClusterRoleBindingkhi delete
Comment on lines
+206
to
+255
| "spec": map[string]interface{}{ | ||
| "backoffLimit": 3, | ||
| "ttlSecondsAfterFinished": 3600, | ||
| "serviceAccountName": fmt.Sprintf("%s-migrator", heliosApp.Name), | ||
| "template": map[string]interface{}{ | ||
| "metadata": map[string]interface{}{ | ||
| "labels": map[string]interface{}{ | ||
| "app": heliosApp.Name, | ||
| "job-type": "db-migration", | ||
| }, | ||
| }, | ||
| "spec": map[string]interface{}{ | ||
| "containers": []map[string]interface{}{ | ||
| { | ||
| "name": "db-migrate", | ||
| "image": migrateImage, | ||
| "imagePullPolicy": "Always", | ||
| "env": []map[string]interface{}{ | ||
| { | ||
| "name": "PGRST_DB_URI", | ||
| "valueFrom": map[string]interface{}{ | ||
| "secretKeyRef": map[string]interface{}{ | ||
| "name": fmt.Sprintf("%s-db-credentials", heliosApp.Name), | ||
| "key": "uri", | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| "resources": map[string]interface{}{ | ||
| "requests": map[string]interface{}{ | ||
| "cpu": "100m", | ||
| "memory": "128Mi", | ||
| }, | ||
| "limits": map[string]interface{}{ | ||
| "cpu": "500m", | ||
| "memory": "512Mi", | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| "restartPolicy": "Never", | ||
| "securityContext": map[string]interface{}{ | ||
| "runAsNonRoot": true, | ||
| "runAsUser": 1000, | ||
| "fsReadOnlyRootFilesystem": true, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| } |
There was a problem hiding this comment.
serviceAccountName được đặt sai vị trí trong Job spec.
serviceAccountName ở Line 209 nằm trong spec của Job thay vì spec.template.spec. Điều này khiến Job sẽ không sử dụng ServiceAccount đã tạo.
Ngoài ra, fsReadOnlyRootFilesystem (Line 250) là field không hợp lệ - tên đúng trong securityContext của container là readOnlyRootFilesystem.
🐛 Đề xuất sửa cấu trúc Job
"spec": map[string]interface{}{
"backoffLimit": 3,
"ttlSecondsAfterFinished": 3600,
- "serviceAccountName": fmt.Sprintf("%s-migrator", heliosApp.Name),
"template": map[string]interface{}{
"metadata": map[string]interface{}{
"labels": map[string]interface{}{
"app": heliosApp.Name,
"job-type": "db-migration",
},
},
"spec": map[string]interface{}{
+ "serviceAccountName": fmt.Sprintf("%s-migrator", heliosApp.Name),
"containers": []map[string]interface{}{
// ... containers config
},
"restartPolicy": "Never",
"securityContext": map[string]interface{}{
"runAsNonRoot": true,
"runAsUser": 1000,
- "fsReadOnlyRootFilesystem": true,
},
},
},
},Lưu ý: readOnlyRootFilesystem nên được đặt trong securityContext của từng container, không phải pod-level.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "spec": map[string]interface{}{ | |
| "backoffLimit": 3, | |
| "ttlSecondsAfterFinished": 3600, | |
| "serviceAccountName": fmt.Sprintf("%s-migrator", heliosApp.Name), | |
| "template": map[string]interface{}{ | |
| "metadata": map[string]interface{}{ | |
| "labels": map[string]interface{}{ | |
| "app": heliosApp.Name, | |
| "job-type": "db-migration", | |
| }, | |
| }, | |
| "spec": map[string]interface{}{ | |
| "containers": []map[string]interface{}{ | |
| { | |
| "name": "db-migrate", | |
| "image": migrateImage, | |
| "imagePullPolicy": "Always", | |
| "env": []map[string]interface{}{ | |
| { | |
| "name": "PGRST_DB_URI", | |
| "valueFrom": map[string]interface{}{ | |
| "secretKeyRef": map[string]interface{}{ | |
| "name": fmt.Sprintf("%s-db-credentials", heliosApp.Name), | |
| "key": "uri", | |
| }, | |
| }, | |
| }, | |
| }, | |
| "resources": map[string]interface{}{ | |
| "requests": map[string]interface{}{ | |
| "cpu": "100m", | |
| "memory": "128Mi", | |
| }, | |
| "limits": map[string]interface{}{ | |
| "cpu": "500m", | |
| "memory": "512Mi", | |
| }, | |
| }, | |
| }, | |
| }, | |
| "restartPolicy": "Never", | |
| "securityContext": map[string]interface{}{ | |
| "runAsNonRoot": true, | |
| "runAsUser": 1000, | |
| "fsReadOnlyRootFilesystem": true, | |
| }, | |
| }, | |
| }, | |
| }, | |
| } | |
| "spec": map[string]interface{}{ | |
| "backoffLimit": 3, | |
| "ttlSecondsAfterFinished": 3600, | |
| "template": map[string]interface{}{ | |
| "metadata": map[string]interface{}{ | |
| "labels": map[string]interface{}{ | |
| "app": heliosApp.Name, | |
| "job-type": "db-migration", | |
| }, | |
| }, | |
| "spec": map[string]interface{}{ | |
| "serviceAccountName": fmt.Sprintf("%s-migrator", heliosApp.Name), | |
| "containers": []map[string]interface{}{ | |
| { | |
| "name": "db-migrate", | |
| "image": migrateImage, | |
| "imagePullPolicy": "Always", | |
| "env": []map[string]interface{}{ | |
| { | |
| "name": "PGRST_DB_URI", | |
| "valueFrom": map[string]interface{}{ | |
| "secretKeyRef": map[string]interface{}{ | |
| "name": fmt.Sprintf("%s-db-credentials", heliosApp.Name), | |
| "key": "uri", | |
| }, | |
| }, | |
| }, | |
| }, | |
| "resources": map[string]interface{}{ | |
| "requests": map[string]interface{}{ | |
| "cpu": "100m", | |
| "memory": "128Mi", | |
| }, | |
| "limits": map[string]interface{}{ | |
| "cpu": "500m", | |
| "memory": "512Mi", | |
| }, | |
| }, | |
| }, | |
| }, | |
| "restartPolicy": "Never", | |
| "securityContext": map[string]interface{}{ | |
| "runAsNonRoot": true, | |
| "runAsUser": 1000, | |
| }, | |
| }, | |
| }, | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/operator/internal/controller/argocd/presync.go` around lines 206 - 255,
The Job manifest incorrectly places serviceAccountName at the Job spec root
(symbol: "serviceAccountName") instead of inside the pod template spec; move
serviceAccountName into the nested "template" -> "spec" map so the pod uses the
created ServiceAccount (e.g., the fmt.Sprintf("%s-migrator", heliosApp.Name)
value). Also fix the container securityContext by renaming the invalid
"fsReadOnlyRootFilesystem" to the correct container-level field
"readOnlyRootFilesystem" inside the container's "securityContext" (symbol:
"securityContext") and ensure it remains under the container spec, not the
pod-level spec.
| echo "Migrations completed successfully" | ||
| ``` | ||
|
|
||
| The `PGRST_DB_URI` environment variable is injected from the `<app>-db-credentials` Secret (managed by the operator's database trait). |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Tìm quy ước đặt tên cho database secret
# Kiểm tra CRD schema
rg -C3 "databaseSecretRef|db-secret|db-credentials" apps/operator/config/crd/
# Kiểm tra trong code operator
rg -C3 "db-secret|db-credentials" apps/operator/internal/controller/Repository: helios-platform-team/helios-platform
Length of output: 18297
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect docs/OPERATOR.md around the referenced line
echo "=== docs/OPERATOR.md:257 ==="
nl -ba docs/OPERATOR.md | sed -n '240,275p'
# Search for db-credentials and db-secret usage in docs
echo "=== Occurrences in docs ==="
rg -n "db-credentials|db-secret" docs/OPERATOR.md docs -S || true
# Search across repo for where PGRST_DB_URI is documented/constructed
echo "=== PGRST_DB_URI references ==="
rg -n "PGRST_DB_URI" -S . || true
# Search for '-db-credentials' secret creation usage
echo "=== -db-credentials references ==="
rg -n "\"%s-db-credentials\"|db-credentials" -S apps/operator . || true
# Search for databaseSecretRef / -db-secret usage
echo "=== -db-secret references ==="
rg -n "databaseSecretRef|GetDatabaseSecretName| -db-secret\b|db-secret\"" -S apps/operator/config apps/operator/internal/controller . || trueRepository: helios-platform-team/helios-platform
Length of output: 170
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== docs/OPERATOR.md: lines 240-275 (no nl) ==="
sed -n '240,275p' docs/OPERATOR.md
echo "=== Occurrences in docs ==="
rg -n "db-credentials|db-secret" docs/OPERATOR.md docs -S || true
echo "=== PGRST_DB_URI references ==="
rg -n "PGRST_DB_URI" -S . || true
echo "=== -db-credentials references (operator) ==="
rg -n "\"%s-db-credentials\"|db-credentials" -S apps/operator . || true
echo "=== databaseSecretRef / -db-secret references (operator) ==="
rg -n "databaseSecretRef|GetDatabaseSecretName| -db-secret\\b|\\-db-secret" -S apps/operator/config apps/operator/internal/controller . || trueRepository: helios-platform-team/helios-platform
Length of output: 23446
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search operator for '-db-credentials' creation ==="
rg -n "\"%s-db-credentials\"|db-credentials|DB_CREDENTIALS" -S apps/operator/internal/controller apps/operator/api apps/operator/config apps/operator/internal/cue . || true
echo "=== Inspect resources.go for secret naming ==="
rg -n "PGRST_DB_URI|db-credentials|-db-credentials" -S apps/operator/internal/controller/database/resources.go
sed -n '1,220p' apps/operator/internal/controller/database/resources.go || true
echo "=== Inspect injection.go to see how PGRST_DB_URI is stored ==="
rg -n "PGRST_DB_URI" -S apps/operator/internal/controller/database/injection.go
sed -n '1,220p' apps/operator/internal/controller/database/injection.go || trueRepository: helios-platform-team/helios-platform
Length of output: 13127
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== docs/OPERATOR.md around 240-265 for context ==="
sed -n '220,270p' docs/OPERATOR.md
echo "=== Inspect apps/operator/internal/controller/argocd/presync.go around db-credentials ==="
sed -n '180,280p' apps/operator/internal/controller/argocd/presync.go
echo "=== Inspect cue/definitions/argocd/pre-sync-job.cue around PGRST_DB_URI/db-credentials ==="
sed -n '1,120p' cue/definitions/argocd/pre-sync-job.cue
sed -n '120,220p' cue/definitions/argocd/pre-sync-job.cue
echo "=== Inspect cue/definitions/tekton/tasks/postgrest-reload.cue for expected secret name/keys ==="
sed -n '1,120p' cue/definitions/tekton/tasks/postgrest-reload.cue
echo "=== Inspect cue/definitions/tekton/tasks/db-migrate.cue for expected secret name/keys ==="
sed -n '1,120p' cue/definitions/tekton/tasks/db-migrate.cueRepository: helios-platform-team/helios-platform
Length of output: 12245
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for secretKeyRef key: \"uri\" ==="
rg -n "secretKeyRef:.*\"uri\"|secretKeyRef.*key:\s*\"uri\"|\bkey:\s*\"uri\"|\"uri\"\s*\}," -S apps/operator . || true
echo "=== Search for data[\"uri\"] or map keys 'uri' in secret generation ==="
rg -n "Data:\s*map\\[string\\].*\\{\"uri\"|\"uri\"\\s*:\\s*\\[\\]byte|\\buri\\b.*\\bSecret\\b" -S apps/operator/internal/controller . || true
echo "=== Search for secret-type database-credentials generator ==="
rg -n "secret-type\"\\s*:\\s*\"database-credentials\"|database-credentials" -S apps/operator/internal/controller . || true
echo "=== Search for GenerateDatabaseSecret usage / naming ==="
rg -n "GenerateDatabaseSecret\\(" -S apps/operator/internal/controller . || trueRepository: helios-platform-team/helios-platform
Length of output: 2224
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for 'db-credentials' in repo ==="
rg -n "db-credentials" -S . || true
echo "=== Inspect apps/portal/examples/postgrest-template/MIGRATION_SETUP.md around db-credentials ==="
sed -n '130,210p' apps/portal/examples/postgrest-template/MIGRATION_SETUP.md
echo "=== Inspect apps/portal/examples/postgrest-template/README.md around the secret verification section ==="
rg -n "db-credentials" apps/portal/examples/postgrest-template/README.md -n || true
sed -n '600,720p' apps/portal/examples/postgrest-template/README.md
echo "=== Inspect docs/manual-verification-guide.md around db-secret (if it mentions PGRST_DB_URI/uri) ==="
rg -n "db-secret|PGRST_DB_URI|PGRST_DB_URI.*uri|uri" docs/manual-verification-guide.md -S || true
sed -n '350,580p' docs/manual-verification-guide.mdRepository: helios-platform-team/helios-platform
Length of output: 11030
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Inspect cue/definitions/traits/database.cue (secret naming inputs) ==="
sed -n '1,120p' cue/definitions/traits/database.cue
sed -n '120,240p' cue/definitions/traits/database.cue
echo "=== Inspect apps/operator/api/v1alpha1/heliosapp_types.go for DatabaseSecretRef semantics ==="
sed -n '90,180p' apps/operator/api/v1alpha1/heliosapp_types.go
echo "=== Inspect apps/operator/internal/cue/tekton.go for databaseSecretRef mapping ==="
sed -n '1,120p' apps/operator/internal/cue/tekton.go
echo "=== Inspect apps/operator/internal/controller/tekton/mapper.go for DatabaseSecretRef default ==="
sed -n '1,120p' apps/operator/internal/controller/tekton/mapper.goRepository: helios-platform-team/helios-platform
Length of output: 11938
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for ComponentName usage in database reconciler ==="
rg -n "ComponentName" apps/operator/internal/controller/database -S
echo "=== Inspect apps/operator/internal/controller/database/traits.go fully ==="
sed -n '1,220p' apps/operator/internal/controller/database/traits.go
echo "=== Inspect apps/operator/internal/controller/database/reconciler.go sections where secretName is chosen ==="
rg -n "GetDatabaseSecretName\\(|DatabaseSecretRef|secretName :=" apps/operator/internal/controller/database/reconciler.go
sed -n '1,220p' apps/operator/internal/controller/database/reconciler.go
sed -n '220,380p' apps/operator/internal/controller/database/reconciler.go
echo "=== Search for databaseSecretRef usage in presync/job generation ==="
rg -n "databaseSecretRef|DatabaseSecretRef" apps/operator/internal/controller/argocd/presync.go -SRepository: helios-platform-team/helios-platform
Length of output: 21210
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== argocd presync.go: all db-credentials/uri occurrences ==="
rg -n "db-credentials|key\\\":\\s*\"uri\"|\"uri\"|PGRST_DB_URI|PGRST" apps/operator/internal/controller/argocd/presync.go
echo "=== operator argocd controller: search for PGRST_DB_URI / uri usage ==="
rg -n "PGRST_DB_URI|\"uri\"|db-credentials" apps/operator/internal/controller/argocd -S
echo "=== operator: search for secrets with key PGRST_DB_URI in secret generators ==="
rg -n "PGRST_DB_URI" apps/operator/internal/controller -S
echo "=== operator: search for GenerateDatabaseSecret labels/name patterns ==="
rg -n "database-credentials|secret-type\"\\s*:\\s*\"database-credentials\"|GenerateDatabaseSecret\\(|-db-secret\\b" apps/operator/internal/controller/database -SRepository: helios-platform-team/helios-platform
Length of output: 299
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== argocd/presync.go: literal occurrences ==="
rg -n "db-credentials" apps/operator/internal/controller/argocd/presync.go
rg -n "PGRST_DB_URI" apps/operator/internal/controller/argocd/presync.go
rg -n 'key"' apps/operator/internal/controller/argocd/presync.go
rg -n "\"uri\"" apps/operator/internal/controller/argocd/presync.go
rg -n '"uri"' apps/operator/internal/controller/argocd/presync.go
echo "=== operator: find any mention of SecretKeyRef key \"uri\" ==="
rg -n "secretKeyRef.*key.*\"uri\"|\"key\":\\s*\"uri\"|\"key\"\\s*:\\s*\"uri\"|\"uri\"" apps/operator/internal/controller -S || true
echo "=== operator: find any mention of PGRST_DB_URI in secret key refs ==="
rg -n "PGRST_DB_URI|\"PGRST_DB_URI\"|key.*PGRST_DB_URI" apps/operator/internal/controller -S || true
echo "=== database reconciler/test: confirm secret data keys ==="
rg -n "\"DB_USER\"|\"DB_PASS\"|\"DB_HOST\"|\"PGRST_DB_URI\"" apps/operator/internal/controller/database -SRepository: helios-platform-team/helios-platform
Length of output: 7870
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for secret data keys \"uri\" in repo ==="
rg -n "\"uri\"\\s*:\\s*\\[\\]byte|\"key\"\\s*:\\s*\"uri\"|Key:\\s*\"uri\"|key:\\s*\"uri\"" -S . || true
echo "=== Search for 'db-credentials' secret templates/creation ==="
rg -n "db-credentials" -S cue apps/operator . || true
echo "=== Search for 'PGRST_DB_URI' vs 'uri' usage ==="
rg -n "PGRST_DB_URI|\"uri\"" -S apps/operator/internal/controller/argocd/presync.go cue/definitions/argocd/pre-sync-job.cue cue/definitions/tekton/tasks/db-migrate.cue cue/definitions/tekton/tasks/postgrest-reload.cue docs/OPERATOR.md || trueRepository: helios-platform-team/helios-platform
Length of output: 2334
Đồng bộ lại tên/key Secret cho PGRST_DB_URI trong tài liệu và operator
docs/OPERATOR.md (dòng 257) ghi PGRST_DB_URI inject từ Secret <app>-db-credentials do database trait quản lý, nhưng code operator lại:
- Trong PreSync job:
apps/operator/internal/controller/argocd/presync.govàcue/definitions/argocd/pre-sync-job.cuetham chiếu Secret<app>-db-credentialsvới key"uri". - Database trait tạo Secret theo quy ước
<component>-db-secret(GetDatabaseSecretName) và lưu connection string ở keyPGRST_DB_URI(apps/operator/internal/controller/database/resources.go). - CRD
databaseSecretRefdefault{appName}-db-secret, nhưng PreSync job hiện không dùng field này.
Cần sửa để thống nhất Secret name và key (hoặc cập nhật lại tài liệu cho đúng cơ chế thực tế).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/OPERATOR.md` at line 257, Docs and operator disagree on the DB secret
name/key: the PreSync job currently looks for "<app>-db-credentials" with key
"uri" while the database trait creates "<component>-db-secret" using key
"PGRST_DB_URI" and exposes a databaseSecretRef default. Update the PreSync
implementation to read the configured databaseSecretRef (use the same naming
produced by GetDatabaseSecretName) and the PGRST_DB_URI key (not "uri"), i.e.
modify the PreSync job templates (pre-sync-job.cue) and presync logic
(presync.go) to respect databaseSecretRef/default and use "PGRST_DB_URI";
alternatively, if you prefer changing the trait, make GetDatabaseSecretName
produce the "<app>-db-credentials" name and write the connection string under
key "uri", and then update docs to match—choose one approach and make docs,
PreSync (presync.go, pre-sync-job.cue), and database secret generation
(GetDatabaseSecretName / resources.go) consistent.
Comment on lines
+282
to
+285
| When the `database` trait is present, the scaffolder automatically includes: | ||
| - PreSync Job configured to use `<org>/my-api-migrate:latest` image | ||
| - ServiceAccount with permissions to run the Job | ||
| - Kustomization to apply all resources |
There was a problem hiding this comment.
Sửa mô tả về nguồn gốc của PreSync Job và RBAC resources.
Đoạn này nói "When the database trait is present, the scaffolder automatically includes" PreSync Job và ServiceAccount. Điều này không chính xác - theo layer 1 của review stack context, đây là các resource do operator tự động tạo ra khi reconcile HeliosApp có database trait, không phải do scaffolder.
🔧 Đề xuất sửa đổi
-When the `database` trait is present, the scaffolder automatically includes:
-- PreSync Job configured to use `<org>/my-api-migrate:latest` image
-- ServiceAccount with permissions to run the Job
-- Kustomization to apply all resources
+When the `database` trait is present, the operator automatically generates:
+- PreSync Job configured to use `<org>/my-api-migrate:latest` image
+- ServiceAccount, Role, and RoleBinding with permissions to run the Job
+- ArgoCD Application with PreSync/PostSync hook annotations📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| When the `database` trait is present, the scaffolder automatically includes: | |
| - PreSync Job configured to use `<org>/my-api-migrate:latest` image | |
| - ServiceAccount with permissions to run the Job | |
| - Kustomization to apply all resources | |
| When the `database` trait is present, the operator automatically generates: | |
| - PreSync Job configured to use `<org>/my-api-migrate:latest` image | |
| - ServiceAccount, Role, and RoleBinding with permissions to run the Job | |
| - ArgoCD Application with PreSync/PostSync hook annotations |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/OPERATOR.md` around lines 282 - 285, Cập nhật mô tả trong
docs/OPERATOR.md để sửa nguồn gốc của các resource: thay đoạn "When the
`database` trait is present, the scaffolder automatically includes" bằng một câu
rõ ràng nói rằng PreSync Job, ServiceAccount và Kustomization được tạo tự động
bởi operator khi reconcile một HeliosApp có `database` trait (không phải do
scaffolder); tham chiếu rõ các tên phần tử là PreSync Job, ServiceAccount và các
RBAC resources để tránh nhầm lẫn.
Comment on lines
+100
to
+111
| ### Step 2.2: Verify Operator Generated PreSync Resources | ||
|
|
||
| ```bash | ||
| # Check ServiceAccount was auto-created | ||
| kubectl get sa -n <namespace> | grep migrator | ||
|
|
||
| # Verify ClusterRole for Job management | ||
| kubectl get clusterrole | grep presync-job-role | ||
|
|
||
| # Verify ClusterRoleBinding | ||
| kubectl get clusterrolebinding | grep presync-job-binding | ||
| ``` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Kiểm tra naming convention cho RBAC resources
# Tìm code tạo ServiceAccount
rg -C5 "ServiceAccount.*Name|migrator" apps/operator/internal/controller/argocd/presync.go
# Tìm code tạo Role/RoleBinding
rg -C5 "ClusterRole.*Name|presync.*role" apps/operator/internal/controller/argocd/presync.goRepository: helios-platform-team/helios-platform
Length of output: 2008
Điều chỉnh ví dụ kiểm tra RBAC theo naming convention thực tế của operator
ServiceAccountoperator tạo:fmt.Sprintf("%s-migrator", heliosApp.Name)⇒ hướng dẫn nên grep theo"<HeliosAppName>-migrator"thay vì chỉmigrator.ClusterRoleoperator tạo:fmt.Sprintf("%s-presync-job-role", heliosApp.Name)⇒ hướng dẫn nên grep theo"<HeliosAppName>-presync-job-role"thay vì chỉpresync-job-role.ClusterRoleBindingcũng cần có prefix"<HeliosAppName>-"; cập nhật lệnh grep theo đúng suffix mà operator đặt trongapps/operator/internal/controller/argocd/presync.go.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/POSTGREST_MIGRATION_TEST_GUIDE.md` around lines 100 - 111, Update the
RBAC verification examples to match the operator's naming convention: use the
HeliosApp name prefix when grepping for ServiceAccount, ClusterRole and
ClusterRoleBinding (they are created with fmt.Sprintf("%s-migrator",
heliosApp.Name), fmt.Sprintf("%s-presync-job-role", heliosApp.Name) and the
similarly prefixed binding in
apps/operator/internal/controller/argocd/presync.go); replace the generic
"migrator", "presync-job-role" and "presync-job-binding" grep patterns with
"<HeliosAppName>-migrator", "<HeliosAppName>-presync-job-role" and the
corresponding "<HeliosAppName>-presync-job-binding" patterns so examples reflect
actual resource names.
Comment on lines
+113
to
+121
| ### Step 2.3: Check Tekton EventListener (Auto-Generated) | ||
|
|
||
| ```bash | ||
| # Once HeliosApp is created, operator should create EventListener | ||
| kubectl get eventlistener -n <namespace> | grep test-api | ||
|
|
||
| # Find the external URL | ||
| kubectl get svc -n el-services | grep test-api-db-migrate-listener | ||
| ``` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Kiểm tra namespace của EventListener service
# Tìm trong CUE definitions
rg -C5 "EventListener|el-services" cue/definitions/tekton/
# Kiểm tra Tekton defaults
kubectl get svc --all-namespaces | grep -i eventlistenerRepository: helios-platform-team/helios-platform
Length of output: 4671
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Xem ngữ cảnh đoạn doc quanh line 113-121
sed -n '90,140p' docs/POSTGREST_MIGRATION_TEST_GUIDE.md | nl -ba | sed -n '1,120p'
# 2) Tìm chỗ bundleParams.namespace được khai báo/đặt trong CUE
rg -n "bundleParams\.namespace|namespace:\s*bundleParams\.namespace|bundleParams" cue/definitions/tekton cue | head -n 200
# 3) Tìm chỗ el-services xuất hiện trong cue và docs
rg -n "el-services" -S . | head -n 200
# 4) Xem các cue liên quan đến db-migrate-trigger/github-push để biết namespace truyền vào từ đâu
for f in cue/definitions/tekton/triggers/db-migrate-trigger.cue cue/definitions/tekton/triggers/github-push.cue; do
if [ -f "$f" ]; then
echo "==== $f ===="
sed -n '1,220p' "$f" | nl -ba | sed -n '1,220p'
fi
doneRepository: helios-platform-team/helios-platform
Length of output: 123
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '90,140p' docs/POSTGREST_MIGRATION_TEST_GUIDE.md | nl -ba
rg -n "bundleParams\.namespace|namespace:\s*bundleParams\.namespace|bundleParams" cue/definitions/tekton cue | head -n 200
rg -n "el-services" -S . | head -n 200
for f in cue/definitions/tekton/triggers/db-migrate-trigger.cue cue/definitions/tekton/triggers/github-push.cue; do
if [ -f "$f" ]; then
echo "==== $f ===="
sed -n '1,220p' "$f" | nl -ba | sed -n '1,220p'
fi
doneRepository: helios-platform-team/helios-platform
Length of output: 123
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Xem ngữ cảnh đoạn doc quanh line 113-121
sed -n '100,140p' docs/POSTGREST_MIGRATION_TEST_GUIDE.md | nl -ba
# 2) Tìm chỗ bundleParams.namespace được khai báo/đặt trong CUE
rg -n "bundleParams\.namespace|namespace:\s*bundleParams\.namespace|bundleParams" cue | head -n 200
# 3) Tìm chỗ el-services xuất hiện trong cue và docs
rg -n "el-services" -S . | head -n 200
# 4) Xem cue template liên quan đến EventListener (đoạn nơi gán namespace)
for f in cue/definitions/tekton/base_trigger.cue cue/definitions/tekton/triggers/db-migrate-trigger.cue cue/definitions/tekton/triggers/github-push.cue; do
if [ -f "$f" ]; then
echo "==== $f (relevant excerpt) ===="
rg -n "EventListener|namespace:\s*parameter\.namespace|bundleParams\.namespace" "$f" || true
echo "----"
sed -n '1,220p' "$f" | nl -ba | sed -n '1,220p'
fi
doneRepository: helios-platform-team/helios-platform
Length of output: 123
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '100,140p' docs/POSTGREST_MIGRATION_TEST_GUIDE.md | nl -ba
rg -n "bundleParams\.namespace|namespace:\s*bundleParams\.namespace|bundleParams" cue | head -n 200
rg -n "el-services" -S . | head -n 200
for f in cue/definitions/tekton/base_trigger.cue cue/definitions/tekton/triggers/db-migrate-trigger.cue cue/definitions/tekton/triggers/github-push.cue; do
if [ -f "$f" ]; then
echo "==== $f (relevant excerpt) ===="
rg -n "EventListener|namespace:" "$f" || true
echo "----"
sed -n '1,240p' "$f" | nl -ba | sed -n '1,240p'
fi
doneRepository: helios-platform-team/helios-platform
Length of output: 123
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "### docs excerpt"
sed -n '100,140p' docs/POSTGREST_MIGRATION_TEST_GUIDE.md | nl -ba
echo "### Search bundleParams.namespace usage"
rg -n "bundleParams\.namespace|namespace:\s*bundleParams\.namespace|bundleParams" cue | head -n 200
echo "### Search el-services usage"
rg -n "el-services" -S . | head -n 200
echo "### Inspect relevant CUE files for namespace wiring"
for f in cue/definitions/tekton/base_trigger.cue cue/definitions/tekton/triggers/db-migrate-trigger.cue cue/definitions/tekton/triggers/github-push.cue; do
if [ -f "$f" ]; then
echo "==== $f ===="
rg -n "EventListener|bundleParams\.namespace|parameter\.namespace|namespace:" "$f" || true
echo "---- top excerpt ----"
sed -n '1,200p' "$f" | nl -ba
echo
fi
doneRepository: helios-platform-team/helios-platform
Length of output: 158
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "### docs excerpt"
sed -n '100,140p' docs/POSTGREST_MIGRATION_TEST_GUIDE.md | nl -ba
echo "### Search bundleParams.namespace usage"
rg -n "bundleParams\.namespace|namespace:\s*bundleParams\.namespace|bundleParams" cue | head -n 200
echo "### Search el-services usage"
rg -n "el-services" -S . | head -n 200
echo "### Inspect relevant CUE files for namespace wiring"
for f in cue/definitions/tekton/base_trigger.cue cue/definitions/tekton/triggers/db-migrate-trigger.cue cue/definitions/tekton/triggers/github-push.cue; do
if [ -f "$f" ]; then
echo "==== $f ===="
rg -n "EventListener|bundleParams\.namespace|parameter\.namespace|namespace:" "$f" || true
echo "---- top excerpt ----"
sed -n '1,200p' "$f" | nl -ba
echo
fi
doneRepository: helios-platform-team/helios-platform
Length of output: 158
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "### docs excerpt (line 100-140)";
sed -n '100,140p' docs/POSTGREST_MIGRATION_TEST_GUIDE.md | cat -n
echo "### rg bundleParams.namespace in cue";
rg -n "bundleParams\.namespace|namespace:\s*bundleParams\.namespace" cue | head -n 200
echo "### rg el-services in repo";
rg -n "el-services" -S . | head -n 200
echo "### inspect namespace wiring in cue/definitions/tekton/base_trigger.cue (EventListener template)";
rg -n "bundleParams\.namespace|parameter\.namespace|`#TektonEventListener`|EventListener" cue/definitions/tekton/base_trigger.cue
echo "---- context around parameter.namespace ----";
rg -n "parameter\.namespace" cue/definitions/tekton/base_trigger.cue -n -C2 || true
echo "### inspect trigger files for bundleParams.namespace usage";
for f in cue/definitions/tekton/triggers/db-migrate-trigger.cue cue/definitions/tekton/triggers/github-push.cue; do
if [ -f "$f" ]; then
echo "==== $f ===="
rg -n "EventListener|bundleParams\.namespace|parameter\.namespace" "$f" -n || true
fi
doneRepository: helios-platform-team/helios-platform
Length of output: 3850
Đồng bộ namespace cho Tekton EventListener: không nên hardcode el-services nếu CUE dùng bundleParams.namespace
Trong CUE, EventListener được tạo ở namespace theo bundleParams.namespace (template #TektonEventListener dùng metadata.namespace: parameter.namespace, và các trigger gán namespace: bundleParams.namespace trong cue/definitions/tekton/triggers/db-migrate-trigger.cue, cue/definitions/tekton/triggers/github-push.cue, cue/definitions/tekton/triggers/registry.cue). Vì vậy ở docs/POSTGREST_MIGRATION_TEST_GUIDE.md (Step 2.3) đang dùng <namespace> cho kubectl get eventlistener nhưng lại hardcode -n el-services cho kubectl get svc ... cần sửa để dùng cùng một namespace (-n <namespace>) hoặc nêu rõ lý do vì sao trong test này bundleParams.namespace luôn là el-services.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/POSTGREST_MIGRATION_TEST_GUIDE.md` around lines 113 - 121, The docs
hardcode the service namespace (-n el-services) in Step 2.3 which conflicts with
CUE's bundleParams.namespace used when creating the Tekton EventListener (see
template `#TektonEventListener` and bundleParams.namespace in
cue/definitions/tekton/triggers/*.cue); update the example in
docs/POSTGREST_MIGRATION_TEST_GUIDE.md so both kubectl commands use the same
namespace placeholder (-n <namespace>) or add a short note explaining why this
test uniquely requires el-services and that bundleParams.namespace is otherwise
used. Ensure references to the EventListener lookup and the svc lookup are
consistent with bundleParams.namespace.
Comment on lines
+287
to
+295
| # Example successful output: | ||
| # flyway Validate | ||
| # flyway Repair | ||
| # Repairing schema history table in schema [public]... | ||
| # Repairing successful. | ||
| # flyway Migrate | ||
| # Migrating schema [public] to version 1 - init | ||
| # Successfully applied 1 migration to schema [public] | ||
| ``` |
There was a problem hiding this comment.
Sửa expected output - dùng golang-migrate chứ không phải Flyway.
Phần "Example successful output" hiển thị output của Flyway:
flyway Validate
flyway Repair
...
flyway Migrate
Nhưng theo documentation, migration flow sử dụng golang-migrate, không phải Flyway. Expected output nên là:
Running database migrations...
1/u init (42.123ms)
Migrations completed successfully
🔧 Đề xuất sửa expected output
# Expected output should show:
# - Connecting to database
# - Running migrations
-# - "1 migration(s) applied"
+# - Migration version and timing information
# Example successful output:
-# flyway Validate
-# flyway Repair
-# Repairing schema history table in schema [public]...
-# Repairing successful.
-# flyway Migrate
-# Migrating schema [public] to version 1 - init
-# Successfully applied 1 migration to schema [public]
+# Running database migrations...
+# 1/u init (42.123ms)
+# Migrations completed successfully🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/POSTGREST_MIGRATION_TEST_GUIDE.md` around lines 287 - 295, The "Example
successful output" block currently shows Flyway logs; update that block to
reflect golang-migrate output instead. Replace the Flyway lines (the fenced code
block under "Example successful output") with the golang-migrate lines: "Running
database migrations...", "1/u init (42.123ms)", and "Migrations completed
successfully" so the expected output matches the project's migration flow.
added 3 commits
May 26, 2026 22:57
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/operator/config/rbac/role.yaml (1)
73-84:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThu hẹp quyền RBAC cụm để tránh rủi ro leo thang đặc quyền.
Rule này cấp quyền ghi đầy đủ lên
clusterrolesvàclusterrolebindings, làm bề mặt tấn công của controller quá rộng. Nên chuyển sang mô hình least-privilege (ví dụ dùng ClusterRole dùng chung được cấp phát ngoài reconcile và chỉ bind phạm vi cần thiết).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/operator/config/rbac/role.yaml` around lines 73 - 84, Hiện tại rule đang cấp quyền ghi toàn bộ lên clusterroles và clusterrolebindings (và các verb create/delete/update/patch) — thu hẹp quyền bằng cách loại bỏ các verb ghi trên resource clusterroles và clusterrolebindings trong role này, chỉ giữ các verb cần thiết như get/list/watch cho việc reconcile; nếu controller cần tạo hoặc chỉnh sửa ClusterRole/ClusterRoleBinding, tách ra thành một ClusterRole riêng được cấp phát ngoài quy trình reconcile và chỉ bind cho serviceAccount có scope hạn chế; tương tự, hạn chế roles/rolebindings ở namespace cần thiết và chỉ cho phép create/patch/delete khi thật sự bắt buộc (hoặc chuyển sang read-only) để đạt nguyên tắc least-privilege.
♻️ Duplicate comments (5)
apps/portal/examples/postgrest-template/template.yaml (1)
131-131:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winKhông dùng
parameters.namelàm webhook secret.Secret đoán được làm suy yếu xác thực webhook.
Đề xuất chỉnh sửa
- webhookSecret: ${{ parameters.name }} + webhookSecret: ${{ parameters.webhookSecret }} @@ - secret: ${{ parameters.name }} + secret: ${{ parameters.webhookSecret }} @@ - secretToken: ${{ parameters.name }} + secretToken: ${{ parameters.webhookSecret }}- title: Repository & Webhook required: - repoUrl + - webhookSecret properties: @@ + webhookSecret: + title: Webhook Secret + type: string + ui:field: SecretAlso applies to: 186-188
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/portal/examples/postgrest-template/template.yaml` at line 131, Do not use the predictable parameters.name as the webhook secret; replace occurrences of webhookSecret: ${{ parameters.name }} with a reference to a secure secret (e.g., a dedicated parameter like parameters.webhookSecret typed as secureString, or a secret retrieved from your secret store/KeyVault) and ensure all other instances (lines referenced 186-188) are updated the same way so the webhook secret is not guessable or stored in plain parameters.cue/definitions/tekton/triggers/db-migrate-trigger.cue (1)
72-72:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKhông nên hardcode
db-secret-name; cần ưu tiên secret cấu hình.Giá trị hiện tại bỏ qua
databaseSecretRef, dễ fail khi app không dùng tên secret mặc định.Đề xuất chỉnh sửa
- {name: "db-secret-name", value: "\(_bp.appName)-db-secret"}, + {name: "db-secret-name", value: [if _bp.databaseSecretRef != _|_ && _bp.databaseSecretRef != "" {_bp.databaseSecretRef}, "\(_bp.appName)-db-secret"][0]},🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cue/definitions/tekton/triggers/db-migrate-trigger.cue` at line 72, Dòng cấu hình đang hardcode trường name "db-secret-name" với value "\(_bp.appName)-db-secret", bỏ qua bất kỳ secret tùy chỉnh nào; hãy thay bằng tham chiếu đến cấu hình secret nếu có (ví dụ sử dụng _bp.databaseSecretRef hoặc trường tương đương trong manifest) và chỉ dùng "\(_bp.appName)-db-secret" làm fallback khi _bp.databaseSecretRef trống; cụ thể: cập nhật trường value để lấy giá trị từ databaseSecretRef (hoặc từ tên trong object databaseSecretRef.name) và chỉ rơi về chuỗi mặc định nếu không tồn tại.apps/portal/examples/postgrest-template/content/source/Dockerfile.migrate (2)
13-28:⚠️ Potential issue | 🟠 Major | ⚡ Quick winContainer migration đang chạy root user.
PreSync migration không cần đặc quyền root; nên chạy non-root để giảm blast radius.
Đề xuất chỉnh sửa
FROM alpine:3.19 @@ RUN apk add --no-cache postgresql-client bash +RUN addgroup -S migrator && adduser -S migrator -G migrator @@ COPY --from=builder /tmp/migrate /usr/local/bin/migrate @@ RUN echo '#!/bin/bash\nset -e\necho "Running database migrations..."\nmigrate -path /migrations -database "$PGRST_DB_URI" up\necho "Migrations completed successfully"\n' > /entrypoint.sh && chmod +x /entrypoint.sh +RUN chown -R migrator:migrator /migrations /entrypoint.sh /usr/local/bin/migrate +USER migrator🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/portal/examples/postgrest-template/content/source/Dockerfile.migrate` around lines 13 - 28, The container runs migrations as root; create and switch to a non-root user so PreSync jobs run unprivileged: add a non-root user/group (e.g., app), ensure ownership of /migrations and /entrypoint.sh is changed to that user, and set USER to that account before ENTRYPOINT so the migrate invocation in /entrypoint.sh runs unprivileged; update the Dockerfile sections around the COPY db/migrations, creation of /entrypoint.sh, and ENTRYPOINT to add the user (using Alpine addgroup/adduser), chown the relevant paths, and set USER app.
10-10:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThiếu kiểm tra integrity khi tải binary từ internet.
wget | tarbỏ qua checksum/signature verification, tạo rủi ro supply-chain trực tiếp.Đề xuất chỉnh sửa
+ARG MIGRATE_VERSION=v4.17.0 +ARG MIGRATE_SHA256=<official_sha256_here> -RUN wget -qO- https://github.com/golang-migrate/migrate/releases/download/v4.17.0/migrate-linux-amd64.tar.gz | tar xz -C /tmp/ +RUN wget -q -O /tmp/migrate.tar.gz "https://github.com/golang-migrate/migrate/releases/download/${MIGRATE_VERSION}/migrate-linux-amd64.tar.gz" \ + && echo "${MIGRATE_SHA256} /tmp/migrate.tar.gz" | sha256sum -c - \ + && tar xzf /tmp/migrate.tar.gz -C /tmp/ \ + && rm -f /tmp/migrate.tar.gz🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/portal/examples/postgrest-template/content/source/Dockerfile.migrate` at line 10, The RUN line that pipes the migrate release directly into tar ("RUN wget -qO- https://github.com/golang-migrate/migrate/releases/download/v4.17.0/migrate-linux-amd64.tar.gz | tar xz -C /tmp/") lacks integrity verification; change it to first download the migrate-linux-amd64.tar.gz and its corresponding checksum or signature, verify the checksum (e.g., sha256sum -c) or GPG signature before extraction, and fail the build if verification fails so only verified archives are extracted to /tmp.cue/definitions/tekton/pipelines/patterns.cue (1)
199-200:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTránh tag mutable
latestvà hardcode build context cho migration image.Cấu hình hiện tại làm build không deterministic và có thể sai context khi repo dùng subpath.
Đề xuất chỉnh sửa
- {name: tekton.#CommonParams.image.name.name, value: "$(params.\(`#PipelineParams.imageRepo.name`))-migrate:latest"}, - {name: tekton.#CommonParams.image.contextSubpath.name, value: "."}, + {name: tekton.#CommonParams.image.name.name, value: "$(params.\(`#PipelineParams.imageRepo.name`))-migrate:$(params.\(`#PipelineParams.appRepoRevision.name`))"}, + {name: tekton.#CommonParams.image.contextSubpath.name, value: "$(params.\(`#PipelineParams.contextSubpath.name`))"},🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cue/definitions/tekton/pipelines/patterns.cue` around lines 199 - 200, Thay vì hardcode tag "latest" và context ".", thay đổi giá trị của trường liên quan tới image và context để dùng tham số có sẵn: thay "(params.\(`#PipelineParams.imageRepo.name`))-migrate:latest" bằng một tag xác định (ví dụ sử dụng param imageTag hoặc commit SHA như "$(params.\(`#PipelineParams.imageRepo.name`))-migrate:$(params.\(`#PipelineParams.imageTag.name`))") và thay value của contextSubpath từ "." sang tham số context (ví dụ "$(params.\(`#PipelineParams.image.contextSubpath.name`))" hoặc một biểu thức xây dựng đường dẫn từ param repository subpath); cập nhật các định nghĩa param tương ứng nếu cần (tham chiếu tới tekton.#CommonParams.image.name.name và tekton.#CommonParams.image.contextSubpath.name để tìm chỗ sửa).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/operator/internal/controller/argocd/presync.go`:
- Around line 70-72: The update conflict stems from using a stale heliosApp
copy: you add a finalizer via AddPreSyncFinalizer then later call Update with
heliosApp.DeepCopy(), which can cause "object has been modified" errors; fix by
using the latest resource (re-fetch the HeliosApp before the later Update), or
perform updates with a conflict-safe approach such as retry.RetryOnConflict
around r.Update, or use client.Patch with client.MergeFrom(original) or
controllerutil.CreateOrUpdate so the later update (the code that currently uses
heliosApp.DeepCopy()) operates on a fresh object rather than a stale copy while
referencing the same AddPreSyncFinalizer/Update call sites.
- Around line 126-152: The reconcileRole function currently creates a
cluster-scoped ClusterRole (and the related ClusterRoleBinding later) granting
broad cluster permissions; change these to namespace-scoped Role and RoleBinding
scoped to heliosApp.Namespace: replace rbacv1.ClusterRole with rbacv1.Role in
reconcileRole (keep the same Rules but scoped to the namespace), set
ObjectMeta.Namespace = heliosApp.Namespace and use a name like
fmt.Sprintf("%s-presync-job-role", heliosApp.Name); likewise update the binding
creation (the code that references ClusterRoleBinding) to create an
rbacv1.RoleBinding that references the Role by name, limits Subjects to the
specific ServiceAccount used by the presync job, and preserves labels and
ownerReferences so garbage collection still works. Ensure all places referencing
ClusterRole/ClusterRoleBinding (including reconcileRole and the binding
function) are updated to the Role/RoleBinding types and namespace usage.
- Around line 240-242: The ArgoCD hook keys are incorrectly placed under
metadata.labels; locate the metadata.labels map where the keys
"argocd.argoproj.io/hook" and "argocd.argoproj.io/hook-deletion-policy" are set
(the map literal that currently contains those entries) and move those two
entries into metadata.annotations instead so ArgoCD recognizes them as hook
annotations (leave other labels unchanged).
In `@apps/operator/internal/controller/database/reconciler.go`:
- Around line 40-41: The code currently collects systemSecrets :=
[]string{"docker-credentials", "helios-gitops-bot"} and then copies them into
every app namespace (lines around the loop that processes namespaces in
reconciler.go), which broadens access; change the logic to stop blind
replication: only copy each secret when the target namespace explicitly requires
it (e.g., check a namespace annotation/label like
app.kubernetes.io/needs-system-secrets or an app-specific allowlist), or bind
access via a ServiceAccount/Role/RoleBinding instead of cloning secrets; update
the code paths that iterate over systemSecrets (the loop handling namespaces
between lines ~42-104) to consult the namespace/app-level predicate before
performing the copy and ensure any new behavior is covered by a unit/integration
test asserting secrets are not present in unaffected namespaces.
- Around line 72-76: The current loop swallows a missing system secret by
logging and doing continue (errors.IsNotFound branch using secretName and
log.Info), which delays failure and obscures root cause; instead, when
errors.IsNotFound(err) for the required system secret, fail fast by returning an
explicit error/result from the reconciler (e.g., return an error or a requeue
with descriptive message) so the Reconcile handler (or calling method in
reconciler.go) surfaces the missing secret immediately; replace the continue
with a return that wraps the missing-secret condition including secretName and
context.
In `@apps/operator/internal/controller/heliosapp_controller.go`:
- Around line 330-333: The deletion path currently calls
preSyncReconciler.HandlePreSyncCleanup and only logs errors then continues
(swallows the error) which can leave the resource stuck; change the flow so that
if preSyncReconciler.HandlePreSyncCleanup returns an error you propagate that
error (return it from the reconcile/delete handler) instead of returning nil,
and only remove the finalizer after HandlePreSyncCleanup succeeds; in other
words, in the delete branch around HandlePreSyncCleanup and the finalizer
removal logic, replace the log-and-continue with returning the error (or requeue
via ctrl.Result plus error) so the controller will retry cleanup until it
succeeds before finalizer removal.
---
Outside diff comments:
In `@apps/operator/config/rbac/role.yaml`:
- Around line 73-84: Hiện tại rule đang cấp quyền ghi toàn bộ lên clusterroles
và clusterrolebindings (và các verb create/delete/update/patch) — thu hẹp quyền
bằng cách loại bỏ các verb ghi trên resource clusterroles và clusterrolebindings
trong role này, chỉ giữ các verb cần thiết như get/list/watch cho việc
reconcile; nếu controller cần tạo hoặc chỉnh sửa ClusterRole/ClusterRoleBinding,
tách ra thành một ClusterRole riêng được cấp phát ngoài quy trình reconcile và
chỉ bind cho serviceAccount có scope hạn chế; tương tự, hạn chế
roles/rolebindings ở namespace cần thiết và chỉ cho phép create/patch/delete khi
thật sự bắt buộc (hoặc chuyển sang read-only) để đạt nguyên tắc least-privilege.
---
Duplicate comments:
In `@apps/portal/examples/postgrest-template/content/source/Dockerfile.migrate`:
- Around line 13-28: The container runs migrations as root; create and switch to
a non-root user so PreSync jobs run unprivileged: add a non-root user/group
(e.g., app), ensure ownership of /migrations and /entrypoint.sh is changed to
that user, and set USER to that account before ENTRYPOINT so the migrate
invocation in /entrypoint.sh runs unprivileged; update the Dockerfile sections
around the COPY db/migrations, creation of /entrypoint.sh, and ENTRYPOINT to add
the user (using Alpine addgroup/adduser), chown the relevant paths, and set USER
app.
- Line 10: The RUN line that pipes the migrate release directly into tar ("RUN
wget -qO-
https://github.com/golang-migrate/migrate/releases/download/v4.17.0/migrate-linux-amd64.tar.gz
| tar xz -C /tmp/") lacks integrity verification; change it to first download
the migrate-linux-amd64.tar.gz and its corresponding checksum or signature,
verify the checksum (e.g., sha256sum -c) or GPG signature before extraction, and
fail the build if verification fails so only verified archives are extracted to
/tmp.
In `@apps/portal/examples/postgrest-template/template.yaml`:
- Line 131: Do not use the predictable parameters.name as the webhook secret;
replace occurrences of webhookSecret: ${{ parameters.name }} with a reference to
a secure secret (e.g., a dedicated parameter like parameters.webhookSecret typed
as secureString, or a secret retrieved from your secret store/KeyVault) and
ensure all other instances (lines referenced 186-188) are updated the same way
so the webhook secret is not guessable or stored in plain parameters.
In `@cue/definitions/tekton/pipelines/patterns.cue`:
- Around line 199-200: Thay vì hardcode tag "latest" và context ".", thay đổi
giá trị của trường liên quan tới image và context để dùng tham số có sẵn: thay
"(params.\(`#PipelineParams.imageRepo.name`))-migrate:latest" bằng một tag xác
định (ví dụ sử dụng param imageTag hoặc commit SHA như
"$(params.\(`#PipelineParams.imageRepo.name`))-migrate:$(params.\(`#PipelineParams.imageTag.name`))")
và thay value của contextSubpath từ "." sang tham số context (ví dụ
"$(params.\(`#PipelineParams.image.contextSubpath.name`))" hoặc một biểu thức xây
dựng đường dẫn từ param repository subpath); cập nhật các định nghĩa param tương
ứng nếu cần (tham chiếu tới tekton.#CommonParams.image.name.name và
tekton.#CommonParams.image.contextSubpath.name để tìm chỗ sửa).
In `@cue/definitions/tekton/triggers/db-migrate-trigger.cue`:
- Line 72: Dòng cấu hình đang hardcode trường name "db-secret-name" với value
"\(_bp.appName)-db-secret", bỏ qua bất kỳ secret tùy chỉnh nào; hãy thay bằng
tham chiếu đến cấu hình secret nếu có (ví dụ sử dụng _bp.databaseSecretRef hoặc
trường tương đương trong manifest) và chỉ dùng "\(_bp.appName)-db-secret" làm
fallback khi _bp.databaseSecretRef trống; cụ thể: cập nhật trường value để lấy
giá trị từ databaseSecretRef (hoặc từ tên trong object databaseSecretRef.name)
và chỉ rơi về chuỗi mặc định nếu không tồn tại.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: aba8dbc2-f06a-4f65-a0f6-b97f6f02a577
📒 Files selected for processing (17)
apps/operator/config/crd/bases/app.helios.io_heliosapps.yamlapps/operator/config/manager/kustomization.yamlapps/operator/config/rbac/role.yamlapps/operator/internal/controller/argocd/application.goapps/operator/internal/controller/argocd/presync.goapps/operator/internal/controller/argocd/reconciler.goapps/operator/internal/controller/database/reconciler.goapps/operator/internal/controller/heliosapp_controller.goapps/operator/internal/controller/interfaces.goapps/portal/examples/postgrest-template/content/source/Dockerfile.migrateapps/portal/examples/postgrest-template/template.yamlcue/definitions/argocd/pre-sync-job.cuecue/definitions/tekton/pipelines/db-migrate-image.cuecue/definitions/tekton/pipelines/patterns.cuecue/definitions/tekton/triggers/db-migrate-trigger.cuedocs/OPERATOR.mddocs/POSTGREST_MIGRATION_TEST_GUIDE.md
Comment on lines
+40
to
+41
| systemSecrets := []string{"docker-credentials", "helios-gitops-bot"} | ||
|
|
There was a problem hiding this comment.
Sao chép secret hệ thống sang mọi namespace làm tăng blast radius quyền truy cập.
Ở Line 40-41 bạn đẩy docker-credentials và helios-gitops-bot cho tất cả app namespace. Đây là mở rộng quyền theo chiều ngang, không theo nguyên tắc least-privilege.
Also applies to: 42-104
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/operator/internal/controller/database/reconciler.go` around lines 40 -
41, The code currently collects systemSecrets := []string{"docker-credentials",
"helios-gitops-bot"} and then copies them into every app namespace (lines around
the loop that processes namespaces in reconciler.go), which broadens access;
change the logic to stop blind replication: only copy each secret when the
target namespace explicitly requires it (e.g., check a namespace
annotation/label like app.kubernetes.io/needs-system-secrets or an app-specific
allowlist), or bind access via a ServiceAccount/Role/RoleBinding instead of
cloning secrets; update the code paths that iterate over systemSecrets (the loop
handling namespaces between lines ~42-104) to consult the namespace/app-level
predicate before performing the copy and ensure any new behavior is covered by a
unit/integration test asserting secrets are not present in unaffected
namespaces.
added 2 commits
May 27, 2026 22:00
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
apps/operator/internal/controller/heliosapp_controller.go (1)
107-112:⚠️ Potential issue | 🟠 Major | ⚡ Quick winProvision system secrets trước bước validate secret references.
validateSecretReferencesđang chạy trước Phase 0.3, trong khi chínhReconcileSystemSecretsmới copyhelios-gitops-botvào namespace app. Với app mới dùng system secret này quaspec.gitOpsSecretRef, reconcile sẽ fail ngay ở pre-flight validation và không bao giờ tới bước provision. Nên đổi thứ tự hai bước này, hoặc exempt các system secret do operator quản lý khỏi validation sớm.Also applies to: 145-149
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/operator/internal/controller/heliosapp_controller.go` around lines 107 - 112, The pre-flight validation calls validateSecretReferences before provisioning operator-managed system secrets via ReconcileSystemSecrets, causing new apps referencing system secrets (e.g., spec.gitOpsSecretRef like "helios-gitops-bot") to fail; change the reconciliation order so ReconcileSystemSecrets runs before validateSecretReferences in the main reconcile flow (or modify validateSecretReferences to skip known operator-managed names/labels), and ensure updateStatus and PhaseFailed are only used after provisioning attempt; locate references to validateSecretReferences and ReconcileSystemSecrets in heliosapp_controller.go and reorder or add an exemption list (operator-managed secret names/labels) so system secrets are available during validation.apps/operator/internal/controller/argocd/presync.go (3)
128-130:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winTạo tên RBAC cluster-scoped theo namespace/UID để tránh đụng tài nguyên.
ClusterRolevàClusterRoleBindingở đây chỉ dùngheliosApp.Name. HaiHeliosAppcùng tên ở namespace khác nhau sẽ cùng ghi vào một resource cluster-scoped: app thứ hai gặpAlreadyExistsrồi tiếp tục với RBAC của app thứ nhất, và cleanup của một app có thể xoá RBAC của app kia. Hãy dùng một helper sinh tên từnamespace + name(hoặcUID) và reuse helper đó cho create/binding/cleanup.Also applies to: 167-179, 365-393
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/operator/internal/controller/argocd/presync.go` around lines 128 - 130, The ClusterRole and ClusterRoleBinding are being named only with heliosApp.Name causing collisions across namespaces; modify the code that builds RBAC names (currently where ClusterRole is created and where ClusterRoleBinding/cleanup reference heliosApp.Name) to use a deterministic helper like buildRBACName(heliosApp.Namespace, heliosApp.Name) or buildRBACNameFromUID(heliosApp.UID) and reuse that helper everywhere (e.g., in the ClusterRole creation, ClusterRoleBinding creation, and cleanup logic referenced around the ClusterRole/ClusterRoleBinding blocks and the code at the other spots noted) so the cluster-scoped RBAC names are unique per app instance and consistent for create/bind/delete.
204-225:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKhông nên chọn DB migrate theo component đầu tiên một cách ngầm định.
Đoạn này lấy component đầu tiên có trait
databaserồi dùng secret của component đó cho toàn bộ PreSync job. Nếu mộtHeliosAppcó nhiều database trait, thứ tự trong spec sẽ quyết định DB nào bị migrate, rất dễ chạy migration vào sai database. Tốt hơn là fail fast khi có nhiều DB target mà chưa có cấu hình explicit cho secret/component đích.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/operator/internal/controller/argocd/presync.go` around lines 204 - 225, The code currently picks the first component with trait database (scanning heliosApp.Spec.Components and using databaseTraitType) and assumes its secret ({componentName}-db-secret) for PreSync; change this to detect multiple database-trait components: collect all components with trait == databaseTraitType, and if more than one is found and there is no explicit configuration specifying which component/secret to use, return an error (fail fast) with a clear message instead of silently using the first; if exactly one is found, continue to set databaseComponentName and databaseSecretName as before; ensure the new validation is performed before any PreSync job creation logic so the operator does not migrate the wrong DB.
51-65:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThiếu nhánh cleanup khi
databasetrait bị gỡ khỏi spec.Nhánh này chỉ
return nilkhi không còn database trait, nhưng không xoá finalizer, RBAC cluster-scoped hay các annotationhelios.io/presync-job/helios.io/has-database-traitđã tạo ở lần reconcile trước. Kết quả là app có thể giữ lại quyền và trạng thái PreSync cũ cho tới lúc bị delete. Nên converge cả chiều ngược lại: khi trait biến mất thì cleanup presync resources và remove finalizer/annotations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/operator/internal/controller/argocd/presync.go` around lines 51 - 65, The current PreSync check only returns when no database trait is found (loop over heliosApp.Spec.Components and trait.Type == databaseTraitType) but does not clean up previously created PreSync resources, finalizer, cluster-scoped RBAC, or annotations (helios.io/presync-job, helios.io/has-database-trait); change the logic in presync.go so that when hasDatabaseTrait == false you run a cleanup path that (1) deletes the Presync Job/Resources previously created, (2) removes the finalizer from the Helios App object, (3) deletes/updates any cluster-scoped RBAC created for the app, and (4) remove the helios.io/presync-job and helios.io/has-database-trait annotations before returning; call the existing helper functions that create/delete these resources (or add tidyDelete helpers if missing) and ensure the same code paths that add finalizers/annotations are mirrored to remove them when trait disappears.apps/operator/internal/controller/database/reconciler.go (1)
50-55:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCần reconcile secret đã tồn tại thay vì bỏ qua hoàn toàn.
Với logic hiện tại, một khi
docker-credentialshoặchelios-gitops-botđã được copy sang namespace app thì mọi lần rotate ở secret nguồn sẽ không bao giờ propagate xuống namespace đó. Các app cũ sẽ giữ credential stale hoặc đã bị revoke. Nên chuyển sang create-or-update cho các secret có nhãn operator-managed, thay vìcontinuengay khi secret đã tồn tại.Also applies to: 81-97
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/operator/internal/controller/database/reconciler.go` around lines 50 - 55, Existing code skips when a secret already exists (err == nil) which prevents propagation of rotations; instead of continuing, implement create-or-update behavior for secrets named by secretName in app.Namespace: detect if the existing Secret has the operator-managed label (e.g., "operator-managed") and if so update its Data/Annotations/Labels to match the source secret (preserving ownerRefs and immutable fields) by using controllerutil.CreateOrUpdate or client.Get + client.Update/ Patch; replace the current continue path with logic that copies/merges the source secret fields and updates the existing Secret, and apply the same change for the similar block referenced at lines 81-97.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/operator/internal/controller/heliosapp_controller_unit_test.go`:
- Around line 201-220: Extract the duplicated Secret fixtures
(dockerCredentialsSecret and heliosGitopsBotSecret) into a single helper
function (e.g., createSystemSecrets(namespace string) []*corev1.Secret) placed
near the top of the test file, returning both secrets configured for the
provided namespace; then update the test functions to call
createSystemSecrets("default") and use the returned slice instead of redefining
dockerCredentialsSecret and heliosGitopsBotSecret inline. Ensure the helper
returns the same Secret metadata, Type, and Data as the originals and update any
test setup that referenced the original variables to use the slice elements or
helper results.
---
Outside diff comments:
In `@apps/operator/internal/controller/argocd/presync.go`:
- Around line 128-130: The ClusterRole and ClusterRoleBinding are being named
only with heliosApp.Name causing collisions across namespaces; modify the code
that builds RBAC names (currently where ClusterRole is created and where
ClusterRoleBinding/cleanup reference heliosApp.Name) to use a deterministic
helper like buildRBACName(heliosApp.Namespace, heliosApp.Name) or
buildRBACNameFromUID(heliosApp.UID) and reuse that helper everywhere (e.g., in
the ClusterRole creation, ClusterRoleBinding creation, and cleanup logic
referenced around the ClusterRole/ClusterRoleBinding blocks and the code at the
other spots noted) so the cluster-scoped RBAC names are unique per app instance
and consistent for create/bind/delete.
- Around line 204-225: The code currently picks the first component with trait
database (scanning heliosApp.Spec.Components and using databaseTraitType) and
assumes its secret ({componentName}-db-secret) for PreSync; change this to
detect multiple database-trait components: collect all components with trait ==
databaseTraitType, and if more than one is found and there is no explicit
configuration specifying which component/secret to use, return an error (fail
fast) with a clear message instead of silently using the first; if exactly one
is found, continue to set databaseComponentName and databaseSecretName as
before; ensure the new validation is performed before any PreSync job creation
logic so the operator does not migrate the wrong DB.
- Around line 51-65: The current PreSync check only returns when no database
trait is found (loop over heliosApp.Spec.Components and trait.Type ==
databaseTraitType) but does not clean up previously created PreSync resources,
finalizer, cluster-scoped RBAC, or annotations (helios.io/presync-job,
helios.io/has-database-trait); change the logic in presync.go so that when
hasDatabaseTrait == false you run a cleanup path that (1) deletes the Presync
Job/Resources previously created, (2) removes the finalizer from the Helios App
object, (3) deletes/updates any cluster-scoped RBAC created for the app, and (4)
remove the helios.io/presync-job and helios.io/has-database-trait annotations
before returning; call the existing helper functions that create/delete these
resources (or add tidyDelete helpers if missing) and ensure the same code paths
that add finalizers/annotations are mirrored to remove them when trait
disappears.
In `@apps/operator/internal/controller/database/reconciler.go`:
- Around line 50-55: Existing code skips when a secret already exists (err ==
nil) which prevents propagation of rotations; instead of continuing, implement
create-or-update behavior for secrets named by secretName in app.Namespace:
detect if the existing Secret has the operator-managed label (e.g.,
"operator-managed") and if so update its Data/Annotations/Labels to match the
source secret (preserving ownerRefs and immutable fields) by using
controllerutil.CreateOrUpdate or client.Get + client.Update/ Patch; replace the
current continue path with logic that copies/merges the source secret fields and
updates the existing Secret, and apply the same change for the similar block
referenced at lines 81-97.
In `@apps/operator/internal/controller/heliosapp_controller.go`:
- Around line 107-112: The pre-flight validation calls validateSecretReferences
before provisioning operator-managed system secrets via ReconcileSystemSecrets,
causing new apps referencing system secrets (e.g., spec.gitOpsSecretRef like
"helios-gitops-bot") to fail; change the reconciliation order so
ReconcileSystemSecrets runs before validateSecretReferences in the main
reconcile flow (or modify validateSecretReferences to skip known
operator-managed names/labels), and ensure updateStatus and PhaseFailed are only
used after provisioning attempt; locate references to validateSecretReferences
and ReconcileSystemSecrets in heliosapp_controller.go and reorder or add an
exemption list (operator-managed secret names/labels) so system secrets are
available during validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4b108316-691c-4792-88f3-4a7d425e5ec2
📒 Files selected for processing (4)
apps/operator/internal/controller/argocd/presync.goapps/operator/internal/controller/database/reconciler.goapps/operator/internal/controller/heliosapp_controller.goapps/operator/internal/controller/heliosapp_controller_unit_test.go
Comment on lines
+201
to
+220
| dockerCredentialsSecret := &corev1.Secret{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "docker-credentials", | ||
| Namespace: "default", | ||
| }, | ||
| Type: corev1.SecretTypeDockercfg, | ||
| Data: map[string][]byte{ | ||
| ".dockercfg": []byte(`{}`), | ||
| }, | ||
| } | ||
|
|
||
| heliosGitopsBotSecret := &corev1.Secret{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "helios-gitops-bot", | ||
| Namespace: "default", | ||
| }, | ||
| Data: map[string][]byte{ | ||
| "token": []byte("dummy-token"), | ||
| }, | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Đề xuất refactor: tách Secret fixtures trùng lặp thành helper function.
Hai Secret fixtures (dockerCredentialsSecret và heliosGitopsBotSecret) được định nghĩa giống hệt nhau trong cả hai test function. Điều này vi phạm nguyên tắc DRY và tăng chi phí bảo trì.
♻️ Đề xuất tạo helper function để tái sử dụng
Tạo helper function ở đầu file:
// createSystemSecrets creates the required system secrets for testing
func createSystemSecrets(namespace string) []*corev1.Secret {
return []*corev1.Secret{
{
ObjectMeta: metav1.ObjectMeta{
Name: "docker-credentials",
Namespace: namespace,
},
Type: corev1.SecretTypeDockercfg,
Data: map[string][]byte{
".dockercfg": []byte(`{}`),
},
},
{
ObjectMeta: metav1.ObjectMeta{
Name: "helios-gitops-bot",
Namespace: namespace,
},
Data: map[string][]byte{
"token": []byte("dummy-token"),
},
},
}
}Sau đó trong mỗi test, sử dụng:
- dockerCredentialsSecret := &corev1.Secret{
- ObjectMeta: metav1.ObjectMeta{
- Name: "docker-credentials",
- Namespace: "default",
- },
- Type: corev1.SecretTypeDockercfg,
- Data: map[string][]byte{
- ".dockercfg": []byte(`{}`),
- },
- }
-
- heliosGitopsBotSecret := &corev1.Secret{
- ObjectMeta: metav1.ObjectMeta{
- Name: "helios-gitops-bot",
- Namespace: "default",
- },
- Data: map[string][]byte{
- "token": []byte("dummy-token"),
- },
- }
+ systemSecrets := createSystemSecrets("default")
fakeClient := fake.NewClientBuilder().WithScheme(scheme).
- WithObjects(heliosApp, dockerCredentialsSecret, heliosGitopsBotSecret).
+ WithObjects(append([]client.Object{heliosApp}, toClientObjects(systemSecrets)...)...).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/operator/internal/controller/heliosapp_controller_unit_test.go` around
lines 201 - 220, Extract the duplicated Secret fixtures (dockerCredentialsSecret
and heliosGitopsBotSecret) into a single helper function (e.g.,
createSystemSecrets(namespace string) []*corev1.Secret) placed near the top of
the test file, returning both secrets configured for the provided namespace;
then update the test functions to call createSystemSecrets("default") and use
the returned slice instead of redefining dockerCredentialsSecret and
heliosGitopsBotSecret inline. Ensure the helper returns the same Secret
metadata, Type, and Data as the originals and update any test setup that
referenced the original variables to use the slice elements or helper results.
NgocAnhDo26
approved these changes
May 31, 2026
NgocAnhDo26
deleted the
users/hpn/refactorPostgREST-CI/CD-pipeline-with-ArgoCD-presync-migrations
branch
This was referenced Jun 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit