Repository navigation
feat(chart): set the service info from values - #285
Merged
Merged
Conversation
Render GET /service-info from the new values serviceInfo.* (id, name, description, organization, contactUrl, documentationUrl, createdAt, updatedAt, environment, storage) instead of the file charts/tesk/service-info/service-info.yaml, which needed a checkout of the chart; serviceInfo.existingConfigMap mounts a ConfigMap of the operator's instead. The defaults reproduce the old file, except for the contact and documentation URLs (now this repository and its docs). The chart sets type (TES 1.0.0) and version (tesk-api's image tag) itself. Text fields are quoted, and createdAt and updatedAt are rendered as YAML timestamps and checked, as tesk-api reads them as dates and answers without service info if the file can't be read. The fields required by the GA4GH specification fail if empty. A change restarts the API's pod, and the notes remind operators while the id is the default. Document the values, the removed file and the upgrade. Closes #282 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011FSoqMpmDCfbACLj3uy6xV
Check createdAt and updatedAt against YAML 1.1's timestamp pattern (a two-digit month and day without a time), as SnakeYAML reads other dates as text, which tesk-api can't parse; one schema definition. Decide the mounted ConfigMap in one helper, so that the volume and the chart's ConfigMap agree, and take the service info's version and the image tag from one helper. Escape DEL and NEL in text, and fail on whole numbers too large to be exact. Test the version against the image tag, the timestamps as YAML reads them, and the fields by name; document quoting of numeric text and the unchanged defaults. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011FSoqMpmDCfbACLj3uy6xV
Fail with the value's key on characters that YAML doesn't allow and JSON doesn't escape (C1 controls but NEL, U+FFFE, U+FFFF), and name the key in the error for numbers too large to be exact. Render whole-number image tags from values files as written, for the image and the service info's version alike. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011FSoqMpmDCfbACLj3uy6xV
Contributor
Reviewer's GuideAPIDefine the chart’s public service-info values contract, including validation for timestamps and the other fields.
ImplementationImplement service-info rendering and shared helpers for selecting its ConfigMap, formatting values, and deriving the API image tag.
Use the shared helpers to render or mount the service-info ConfigMap and restart the API when its rendered contents change.
Show an installation reminder while the default service ID is in use, except when an operator supplies an existing ConfigMap.
ConfigurationAdd defaults and operator-facing values for rendering service info or mounting an existing ConfigMap, and remove the chart’s former service-info source file.
DocumentationDocument the service-info values, defaults, example, ConfigMap option, and object-name behavior in the configuration reference.
Update deployment and upgrade guidance to move service-info edits into values or an operator-managed ConfigMap.
Record the removed file as a breaking change and the new values-based service info as a feature for the 0.3.0 release.
TestsCover service-info defaults, value rendering and validation, image-version consistency, ConfigMap selection, checksums, notes, and removal of the old source file.
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 #285 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 2 2
Lines 422 422
=========================================
Hits 422 422 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
List the service info values under Features, and the removed file under Breaking changes, as operators who edited it must move their edits. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011FSoqMpmDCfbACLj3uy6xV
uniqueg
marked this pull request as ready for review
November 11, 2026 00:28
Contributor
Linked issuesChecked at commit b750b7f #282 feat(chart): set the service info from values · 5 of 5 met
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 5 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="charts/tesk/templates/_helpers.tpl" line_range="85-89" />
<code_context>
+{{- if or (kindIs "invalid" $tag) (eq (toString $tag) "") -}}
+{{- $tag = .Chart.AppVersion -}}
+{{- end -}}
+{{- if and (kindIs "float64" $tag) (eq (floor $tag) $tag) -}}
+{{- printf "%.0f" $tag -}}
+{{- else -}}
+{{- toString $tag -}}
+{{- end -}}
+{{- end -}}
+
</code_context>
<issue_to_address>
**Wrong image tag is deployed**
When an unquoted whole-number image tag in a values file is rounded by Helm, `tesk.apiImageTag` formats the rounded float, so the Deployment requests a different image tag and the service-info version reports that rounded tag; the API image cannot pull the requested tag.
Reject numeric image tags that may have lost precision and require them to be supplied as strings.
</issue_to_address>
### Comment 2
<location path="charts/tesk/templates/_helpers.tpl" line_range="364" />
<code_context>
+{{- /* YAML 1.1 timestamps, as SnakeYAML resolves them (two-digit month
+and day without a time); the same pattern as `$defs/timestamp` in
+values.schema.json, which checks them first */ -}}
+{{- $timestamp := "^([0-9]{4}-[0-9]{2}-[0-9]{2}|[0-9]{4}-[0-9]{1,2}-[0-9]{1,2}([Tt]|[ \t]+)[0-9]{1,2}:[0-9]{2}:[0-9]{2}(\\.[0-9]*)?([ \t]*(Z|[-+][0-9]{1,2}(:[0-9]{2})?))?)$" -}}
+{{- range $field := list "createdAt" "updatedAt" -}}
+{{- $value := index $info $field -}}
</code_context>
<issue_to_address>
**Invalid dates change in service info**
When `serviceInfo.createdAt` or `serviceInfo.updatedAt` contains a shape-matching but impossible calendar date or time, `tesk.serviceInfo` accepts the value because `$timestamp` checks only its shape, then emits it unquoted; SnakeYAML normalizes the invalid timestamp and `/service-info` reports a different date from the configured value.
Validate calendar components before emitting the timestamp.
Also at `charts/tesk/templates/_helpers.tpl:367-368`.
</issue_to_address>
### Comment 3
<location path="charts/tesk/templates/common/service-info-cm.yaml" line_range="12" />
<code_context>
data:
-{{ (.Files.Glob "service-info/*").AsConfig | indent 2 }}
+ service-info.yaml: |
+ {{- . | nindent 4 }}
+{{- end }}
</code_context>
<issue_to_address>
**Large values exceed ConfigMap limit**
When A service-info value or collection makes the rendered ConfigMap data exceed 1 MiB, the template inserts service-info values into the ConfigMap without a size check. If the rendered data exceeds Kubernetes’ 1 MiB ConfigMap limit, the API server rejects the ConfigMap and Helm cannot complete the install or upgrade.
Validate the rendered ConfigMap data size and fail with a clear Helm error before Kubernetes rejects the resource.
</issue_to_address>
### Comment 4
<location path="charts/tesk/templates/_helpers.tpl" line_range="424" />
<code_context>
+{{- $value := .value -}}
+{{- $text := toString $value -}}
+{{- if and (kindIs "float64" $value) (eq (floor $value) $value) -}}
+{{- if not (and (lt $value 9007199254740992.0) (gt $value -9007199254740992.0)) -}}
+{{- fail (printf "%s is a number too large to be exact: quote it in the values file or use --set-string" .key) -}}
+{{- end -}}
</code_context>
<issue_to_address>
**Exact boundary values are rejected**
When a text field is supplied as the unquoted number `9007199254740992` or `-9007199254740992`, `tesk.yamlText` uses strict comparisons that reject both exactly representable `float64` boundary values, so Helm fails to render the service-info value.
Allow values equal to either 2^53 boundary in the precision check.
</issue_to_address>
### Comment 5
<location path="charts/tesk/templates/_helpers.tpl" line_range="354" />
<code_context>
+*/}}
+{{- define "tesk.serviceInfo" -}}
+{{- $info := default dict .Values.serviceInfo -}}
+{{- if not (include "tesk.isSet" $info.existingConfigMap) -}}
+{{- $organization := default dict $info.organization -}}
+{{- range $field, $value := dict "id" $info.id "name" $info.name "organization.name" $organization.name "organization.url" $organization.url -}}
</code_context>
<issue_to_address>
**Ignored values still block installation**
When `serviceInfo.existingConfigMap` is set while another `serviceInfo` value is malformed, helm validates `values.schema.json` before rendering, so setting `serviceInfo.existingConfigMap` does not bypass the schema checks on the other `serviceInfo` fields. A malformed value such as `createdAt: yesterday` therefore prevents installation even though the template skips rendering those fields and the documentation says they are ignored.
Make schema validation conditional on `existingConfigMap` being unset, or otherwise avoid validating fields that are documented as ignored in this mode.
</issue_to_address>Sourcery assessment
Approval pending. 5 findings to address first.
Blocking findings: charts/tesk/templates/_helpers.tpl:89, charts/tesk/templates/_helpers.tpl:364, charts/tesk/templates/common/service-info-cm.yaml:12, charts/tesk/templates/_helpers.tpl:424, charts/tesk/templates/_helpers.tpl:354
Fail on dates and times that look like YAML timestamps but don't exist (e.g., 2021-02-30, 24:00:00), which SnakeYAML would read as other ones, and on numeric image tags above 2^53, which Helm would round. Say that the other serviceInfo values are still checked against the schema with an existing ConfigMap. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011FSoqMpmDCfbACLj3uy6xV
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
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 chart renders
GET /service-infofrom values now, so it can be set when installing a packaged chart:serviceInfo.*:id,name,description,organization.name,organization.url,contactUrl,documentationUrl,createdAt,updatedAt,environmentandstorage. They are rendered into the existing ConfigMap<prefix>-service-info, under the same keyservice-info.yamland mount path.serviceInfo.existingConfigMapmounts a ConfigMap of the operator's instead, and the chart then renders none.charts/tesk/service-info/service-info.yamlis removed.type(org.ga4gh,TES,1.0.0) andversion(the tag of tesk-api's image) itself. A new helpertesk.apiImageTaggives the tag to the image and toversionalike.loadAs(TesServiceInfo); a parse error leavesGET /service-infoempty, with probes still passing):tesk.yamlText).createdAtandupdatedAtare unquoted YAML 1.1 timestamps, checked by the schema ($defs/timestamp) and by the template.id,name,organization.*) fail if empty. Empty optional fields are left out.checksum/service-infopod annotation restarts the API on changes, as tesk-api re-reads the file only hourly.serviceInfo.idis the default.serviceInfo.Decisions
serviceInforender the same content as before (see the next point).existingConfigMap.id,name,description,organization,createdAt,environment: devandstorageare unchanged. There are three deliberate differences:contactUrlanddocumentationUrlnow point to this repository's issues and tohttps://tesk.readthedocs.io/.typeandversionas the chart knows them"),type.versionis1.0.0(was1.0), the TES version that tesk-api 1.2.0 implements and reports in its built-in file.versionis the tesk-api image tag,1.2.0by default (was1.0).NOTE:while the id is the default, rather than aWARNING:, since it applies to every default install.docs/releases/0.3.0.mdgets a Features bullet and, as operators who edited the file must move their edits, a Breaking changes bullet. Both are appended at the ends of their sections, away from feat(chart): set the OIDC provider and groups #284's edits.dev, and their changes don't overlap.git merge-treeof the two branches merges cleanly, and the chart tests pass on the merged tree (509 tests at the time of the check).Verification
Local runs with Helm v3.22.1, as pinned in CI:
helm lint charts/tesk --strict: passed.helm templatewith the defaults and with the docs' examples: rendered.REQUIRE_HELM=1 pytest tests/test_chart.py: 470 passed (the dev branch has 425). The new tests cover:versionagainst the image tag, numeric tags,existingConfigMap, the checksum and the NOTES remindercoverage run -m pytest --ignore=tests/test_chart.py: 156 passed, 100% coverage.pre-commit run --all-files: passed.mkdocs build --strict: passed; the anchor#service-info-serviceinfo, which NOTES links to, exists.Chart install). The sandbox blocks the Docker socket and kind isn't installed. CI runs it; it checks thatGET /service-inforeturns anidandtype.group: org.ga4ghfrom a real tesk-api 1.2.0, which exercises the parse.After the Sourcery fixes (33d3871): 478 chart tests passed locally, pre-commit,
helm lint --strictandmkdocs build --strictpassed. CI on 33d3871: all checks passed (CI passed, Chart, Chart install on kind, Lint, Test, Docs, Release readiness, PR title, CodeQL, Codecov, Sourcery, Read the Docs).Review
Full review (Helm chart):
adversarial-reviewerandquality-reviewer, then a second adversarial round. Fixed:existingConfigMapis decided in one helper. Before,falseor0dropped the ConfigMap but kept mounting it. The schema now also requires a string for it.version.2.026111e+07.Sourcery (5 threads, all answered and resolved):
existingConfigMap, the other fields are not used but still checked against the schema.Declined review findings
Sourcery: a size check against the 1 MiB ConfigMap limit. The API server's error is clear, and the chart checks no object sizes elsewhere.
Sourcery: allowing exactly ±2^53. 2^53 + 1 is read as 2^53 too, so the bound must exclude it (as in
tesk.secretValue).The NOTES reminder compares against the literal default id instead of reading
values.yaml, which templates can't do. A test now pins the literal toVALUES, and comments at both places refer to each other.Open questions
None.
Closes #282
🤖 Generated with Claude Code
https://claude.ai/code/session_011FSoqMpmDCfbACLj3uy6xV
Summary by Sourcery
Configure and validate the API service information through Helm values while supporting operator-managed ConfigMaps and automatic pod refreshes.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests:
Chores: