Repository navigation
feat!: follow Helm chart conventions - #109
Merged
Merged
Conversation
Derive all resource names from the release (`fullname`, with `nameOverride` and `fullnameOverride`; MongoDB as `<fullname>-mongodb`), add the standard labels and selector labels per component, and set `db.host` in the config overlay from the MongoDB name. Run the app and MongoDB with restrictive security contexts by default (non-root, seccomp RuntimeDefault, no privilege escalation, all capabilities dropped, read-only root filesystem with writable emptyDir volumes), configurable via values and without fixed IDs on OpenShift. Add `ingress.className` and `ingress.annotations`, fix NOTES, describe the chart, add the new values to the schema, and document names, labels, ingress and security contexts. Closes #82 BREAKING CHANGE: chart version 4.0.0 renames resources and selectors; uninstall and reinstall releases (dump and restore MongoDB data to keep it). `cloud_registry.appName` is removed; use `fullnameOverride`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Refuse upgrades of releases with chart 3 resources (`lookup` of the old `mongodb` PVC or Deployment of the release), which would otherwise delete the old MongoDB volume, and document a data-preserving migration from chart 3 or 2 (dump, uninstall, install, restore). Fail if the app and MongoDB names would be equal, in one `validate` helper together with the `appName` check. Use https in NOTES whenever an Ingress or Route exists, as the config overlay does, with one helper for the Route condition. Reject unknown ingress keys in the values schema. Document that name overrides are install-time only, fix the MongoDB security context example, and note that changed Secrets need a rollout restart. Extend the tests (name override, OpenShift contexts, schema, NOTES). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Helm adds `app.kubernetes.io/managed-by` to every resource it manages, so chart 3 resources have it too and the upgrade guard never fired. Detect them by the missing `app.kubernetes.io/instance` label, which only chart 4 sets, and test with the labels Helm adds. Note in NOTES when the Ingress has no TLS, comment the checks' include site, drop a duplicate mongosh example and tidy up the tests. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Reviewer's GuideThe PR upgrades the Helm chart to convention-compliant 4.0.0 behavior by making resources release-scoped and consistently labeled, adding secure-by-default and OpenShift-aware pod configuration, improving ingress/Route and NOTES handling, and protecting users from unsafe chart 3 migrations with schema validation, documentation, and extensive render-time tests. Sequence diagram for safe chart 3 to chart 4 migrationsequenceDiagram
actor Operator
participant Helm3 as Helm 3 release
participant MongoDB
participant Helm4 as Helm chart 4
Operator->>Helm3: kubectl scale deploy/cloud-registry --replicas=0
Operator->>MongoDB: mongodump --archive
Operator->>Helm3: helm uninstall cloud-registry
Operator->>Helm4: helm install cloud-registry
Helm4->>MongoDB: Create new MongoDB release resources
Operator->>MongoDB: mongorestore --drop --archive
Operator->>Helm4: kubectl rollout restart deploy/cloud-registry
State diagram for chart upgrade validationstateDiagram-v2
[*] --> Render
Render --> ValidateValues
ValidateValues --> Fail: appName set or names collide
ValidateValues --> RenderResources: valid values
RenderResources --> UpgradeCheck: Release.IsUpgrade
RenderResources --> Ready: New install
UpgradeCheck --> Fail: legacy mongodb PVC or Deployment found
UpgradeCheck --> Ready: no legacy resource found
Fail --> [*]
Ready --> [*]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #109 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 9 9
Lines 573 573
=========================================
Hits 573 573 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
uniqueg
marked this pull request as ready for review
October 20, 2026 19:10
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 10 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="deployment/README.md" line_range="232" />
<code_context>
+ the chart's default version (see [Version](#version)); set
+ `mongodb.image` to the dumped version if needed.
+
+6. Install chart version 4 (see [Installation](#installation)) and wait for
+ MongoDB:
+
</code_context>
<issue_to_address>
**Restore races with app writes**
When the chart 4 app runs while `mongorestore --drop` is restoring collections, the app can accept requests and write collections while `mongorestore` replaces them, so writes are lost or the restored data is inconsistent, and requests can fail during the restore.
Keep the app Deployment at zero replicas until the restore finishes, then start it.
Also at `deployment/README.md:239-248`.
</issue_to_address>
### Comment 2
<location path="deployment/README.md" line_range="185" />
<code_context>
+1. Stop the app, so that the data does not change during the dump:
+
+ ```bash
+ kubectl scale deploy/cloud-registry -n <namespace> --replicas=0
+ ```
+
</code_context>
<issue_to_address>
**Dump overlaps app writes**
When an app pod is still processing a write after the scale command returns, `kubectl scale` does not wait for the pod to terminate, so the following `mongodump` can overlap in-flight metadata writes and produce a snapshot that omits or inconsistently captures registrations.
Wait for the app pods to terminate before starting `mongodump`.
</issue_to_address>
### Comment 3
<location path="deployment/templates/cloud-registry-deploy.yaml" line_range="91" />
<code_context>
- name: {{ .Values.cloud_registry.appName }}-config
+ name: {{ include "cloud-registry.fullname" . }}-config
name: config
+ - emptyDir: {}
+ name: tmp
</code_context>
<issue_to_address>
**Temporary files fail to write**
When the app or a library needs to create a temporary file under `/tmp`, the app runs as UID 1000, but the `emptyDir` has no group ownership or permission adjustment, so the app cannot write there and the request fails with a permission error.
Configure the volume’s ownership or permissions so the app’s runtime UID can write to it.
Also at `deployment/values.yaml:72-75`, `deployment/templates/cloud-registry-deploy.yaml:79-80`.
</issue_to_address>
### Comment 4
<location path="deployment/README.md" line_range="193" />
<code_context>
+ ```bash
+ kubectl exec deploy/mongodb -n <namespace> -- sh -c 'mongodump --quiet \
+ -u "$MONGO_INITDB_ROOT_USERNAME" -p "$MONGO_INITDB_ROOT_PASSWORD" \
+ --authenticationDatabase admin --db serviceStore --archive' \
+ > serviceStore.archive
+ ls -l serviceStore.archive
</code_context>
<issue_to_address>
**Custom database data omitted**
When the app’s `config.db.dbs` contains a database other than `serviceStore`, `mongodump --db serviceStore` exports only the default database, so a release configured with a different or additional `config.db.dbs` database migrates without that data; restoring the archive leaves those services’ data missing after the old volume is removed.
Document dumping every database configured under `config.db.dbs` (or make the command use the actual configured database names) and verify the archive contains them before uninstalling.
</issue_to_address>
### Comment 5
<location path="deployment/README.md" line_range="195" />
<code_context>
+ -u "$MONGO_INITDB_ROOT_USERNAME" -p "$MONGO_INITDB_ROOT_PASSWORD" \
+ --authenticationDatabase admin --db serviceStore --archive' \
+ > serviceStore.archive
+ ls -l serviceStore.archive
+ ```
+
</code_context>
<issue_to_address>
**Partial dump passes migration check**
When `mongodump` or `kubectl exec` exits nonzero after writing some archive bytes, `ls -l` only checks that the archive is nonempty, and the shell continues after a failed dump command. The operator can proceed with an incomplete archive and later use `mongorestore --drop`, leaving collections incomplete.
Make a failed dump stop the migration and verify the archive is readable before uninstalling the old release.
</issue_to_address>
### Comment 6
<location path="deployment/templates/_helpers.tpl" line_range="18-19" />
<code_context>
+{{- .Values.fullnameOverride | trunc 63 | trimSuffix "-" -}}
+{{- else -}}
+{{- $name := include "cloud-registry.name" . -}}
+{{- if contains $name .Release.Name -}}
+{{- .Release.Name | trunc 63 | trimSuffix "-" -}}
+{{- else -}}
+{{- printf "%s-%s" .Release.Name $name | trunc 63 | trimSuffix "-" -}}
</code_context>
<issue_to_address>
**Distinct releases share resource names**
When both `prod` and `prod-cloud-registry` are installed in the same namespace, the `contains` branch gives both releases the same fullname, so their app and MongoDB resource names collide and Helm rejects the second install.
Use a suffix-aware check so a chart name embedded in the release name is not treated as an existing chart-name suffix.
</issue_to_address>
### Comment 7
<location path="deployment/templates/_helpers.tpl" line_range="21" />
<code_context>
+{{- if contains $name .Release.Name -}}
+{{- .Release.Name | trunc 63 | trimSuffix "-" -}}
+{{- else -}}
+{{- printf "%s-%s" .Release.Name $name | trunc 63 | trimSuffix "-" -}}
+{{- end -}}
+{{- end -}}
</code_context>
<issue_to_address>
**Long release names collide**
When two distinct release names share the prefix retained by truncation and neither contains the chart name, `cloud-registry.fullname` truncates both generated names to the same value, so their app and MongoDB resource names collide and Helm rejects the second install.
Preserve uniqueness when truncating generated names, for example by adding a collision-resistant suffix before truncation.
Also at `deployment/templates/_helpers.tpl:32`.
</issue_to_address>
### Comment 8
<location path="deployment/templates/_helpers.tpl" line_range="32" />
<code_context>
+rejects the one name that still collides).
+*/}}
+{{- define "cloud-registry.mongodbFullname" -}}
+{{- printf "%s-mongodb" (include "cloud-registry.fullname" . | trunc 55 | trimSuffix "-") -}}
+{{- end -}}
+
</code_context>
<issue_to_address>
**MongoDB names collide after truncation**
When two releases use `fullnameOverride` values with the same first 55 characters but different later characters, `cloud-registry.mongodbFullname` truncates both values to the same base before adding its suffix, so the MongoDB resource names collide and the second release cannot install.
Preserve uniqueness in the MongoDB name when shortening the override, for example by adding a collision-resistant suffix.
Also at `deployment/templates/_helpers.tpl:18-22`.
</issue_to_address>
### Comment 9
<location path="deployment/templates/_helpers.tpl" line_range="15" />
<code_context>
+*/}}
+{{- define "cloud-registry.fullname" -}}
+{{- if .Values.fullnameOverride -}}
+{{- .Values.fullnameOverride | trunc 63 | trimSuffix "-" -}}
+{{- else -}}
+{{- $name := include "cloud-registry.name" . -}}
</code_context>
<issue_to_address>
**String values render as non-strings**
When a valid name or Ingress class is numeric-only or resembles a YAML boolean, the templates emit string values as bare YAML scalars, so values such as `fullnameOverride: "123"`, a numeric release name, or `ingress.className: "true"` are parsed as numbers or booleans and Kubernetes rejects the rendered resources.
Quote interpolated values in resource names, labels, and Ingress fields that must remain strings.
Also at `deployment/templates/_helpers.tpl:6`, `deployment/templates/_helpers.tpl:16`, `deployment/templates/_helpers.tpl:93-94`, `deployment/templates/cloud-registry-ingress.yaml:24-25`, `deployment/templates/cloud-registry-deploy.yaml:4`.
</issue_to_address>
### Comment 10
<location path="tests/test_chart.py" line_range="528" />
<code_context>
+ assert f'curl -X GET "{url}/ga4gh/registry/v1/service-info"' in notes
+ assert ("WARNING" in notes) is not exposed
+ assert ("TLS must be terminated in front" in notes) is tls_note
+ assert (f"kubectl port-forward svc/{APP} 8080:8080" in notes) is not exposed
</code_context>
<issue_to_address>
**Port-forward namespace omission passes**
When the port-forward command omits the release namespace, when NOTES omits `-n <release namespace>` from the port-forward command, `test_notes` still passes because its assertion checks only the command prefix; users whose current kubectl namespace differs from the release namespace get a command targeting the wrong namespace.
Render NOTES with an explicit non-default namespace and assert the complete port-forward command, including `-n <namespace>`.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 10 findings to address first, and the new resource names give MongoDB a different PersistentVolumeClaim, and moving an existing release requires uninstalling the old chart; that can delete its claim and database data unless it is backed up or retained. Reverting the chart cannot recover data already deleted during that migration.
Blocking findings: deployment/README.md:232, deployment/README.md:185, deployment/templates/cloud-registry-deploy.yaml:91, deployment/README.md:193, deployment/README.md:195, and 5 more
Wait for the old app pods to stop before the dump, verify the dump before uninstalling, and keep the new app at 0 replicas during the restore. Quote interpolated names, labels, hosts and the ingress class, so that numeric-looking values stay strings, and test the namespace in NOTES. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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
1 participant
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
The Helm chart follows the usual conventions (#82). Chart version 4.0.0; app version unchanged.
<release>-cloud-registry, or the release name if it contains the chart name; MongoDB as<fullname>-mongodb), withnameOverrideandfullnameOverride(install-time only).cloud_registry.appNameis removed (rendering fails if set). Port names are fixed (http,mongodb).db.hostin the config overlay is set from the MongoDB Service name (config.db.hoststill wins).app.kubernetes.io/name,instance,component,version,managed-by, andhelm.sh/chart); pod templates and selectors use the selector labels only, so chart upgrades don't restart MongoDB.RuntimeDefault, no privilege escalation, all capabilities dropped, read-only root filesystem withemptyDirvolumes for/tmp(app, MongoDB) and/data/configdb(MongoDB). MongoDB runs as999withfsGroup: 999(the image starts as root otherwise); on OpenShift (security.openshift.io/v1served), fixed user and group IDs are omitted. All configurable viapodSecurityContextandsecurityContextvalues.cloud_registry.ingress.classNameandannotations(merged over the cert-manager ones).service-inforequest; a warning with a port forward if neither an Ingress nor a Route is created.cloud-registry.validate): removedappName, colliding app and MongoDB names, and upgrades of chart 3 releases, which would delete the oldmongodbPersistentVolumeClaim and its data (lookupof the release'smongodbPVC or Deployment without chart 4'sapp.kubernetes.io/instancelabel; only when Helm can query the cluster).ingressandingress.httpsrejected.deployment/README.md): "Names and labels", "Ingress", "Security context", and "From chart version 3 or 2": scale down,mongodump, optionally retain the PV, uninstall, install chart 4,mongorestore, restart. Secret changes need a rollout restart.Closes #82.
Breaking changes
helm upgraderefuses), dumping and restoring MongoDB to keep the data.cloud_registry.appNameis removed; usefullnameOverride.Verification
pytest(Helm 3.18.6): 184 passed. New tests cover names (release names, overrides, truncation, collisions, two releases), labels and selectors, security contexts (incl. OpenShift and overrides), writable mounts,db.host, Ingress class and annotations, the Route, NOTES (5 cases), schema cases, and the upgrade guard's detection logic with resources as returned bylookup.helm lint --strict(test values, auth disabled) andhelm template(defaults, no ingress, HTTPS, auth disabled,--is-upgrade, OpenShift APIs): pass.ruff check,ruff format --check,mypy cloud_registry tests: clean.gitleaks dir .: no leaks.ast-grep scan: clean.lookuppath of the upgrade guard (empty inhelm template), the migration commands and the security contexts with the real images are verified by reading the Helm 3.18.6 source and the official image sources only.managed-by), README security context example, install-time overrides, name collisions, NOTES scheme and TLS note, one Route helper,nameOverridetests, migration commands, schema strictness for ingress, test structure. Declined:additionalProperties: falseeverywhere (would also reject valid keys in open objects such as resources; ingress keys are the ones with new names); moving the migration steps to a separate file; a test of thelookupwiring (needs a cluster).dev(with feat!: return 201 Created for new services #110 and feat: let admins verify services #111).Open questions
None.
Merge order
After #110 (#78) and #111 (#64), both merged.
🤖 Generated with Claude Code