Skip to content

feat(helm): add auto-environment values to k8s-reporter chart#1031

Open
mbevc1 wants to merge 7 commits into
mainfrom
20260724_helm_auto_env
Open

feat(helm): add auto-environment values to k8s-reporter chart#1031
mbevc1 wants to merge 7 commits into
mainfrom
20260724_helm_auto_env

Conversation

@mbevc1

@mbevc1 mbevc1 commented Jul 24, 2026

Copy link
Copy Markdown
Contributor
  • Expose the CLI's --auto-environment flag and its companion flags
    (--environment-description, --include-scaling, --exclude-scaling)
    as reporterConfig.* values on the k8s-reporter chart, so the reporter
    CronJob can auto-create Kosli environments (type K8S) on first run
    without editing the chart.
  • Chart version bumped to 2.6.0 and the values table in README regenerated. Also bumped CLI to 2.33.3
  • Remove obsolete conditionals empty spaces (will render smaller output and triggers changes)
  • Follow-up feat: add --auto-environment flag to kosli snapshot #1028

@mbevc1 mbevc1 added enhancement New feature or request helm labels Jul 24, 2026
@mbevc1
mbevc1 enabled auto-merge (squash) July 24, 2026 16:23
@claude

claude Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Claude finished @mbevc1's task in 1m 20s —— View job


PR Review — Helm auto-environment values

  • Gather context (diff vs origin/main, prior review comments)
  • Verify prior review feedback was addressed
  • Review the chart template changes
  • Post review summary

This PR only touches the k8s-reporter Helm chart (no Go code), so the go build / go vet / go mod tidy gate is not applicable here.

✅ Prior review feedback — all addressed

Prior comment Status
Quote the description value (: → invalid args) cronjob.yaml:117 now uses {{ ... | quote }}
includeScaling string-truthiness footgun (--set-string ...=false) cronjob.yaml:119 now guards with kindIs "bool" for a real tri-state
Release sequencing / v2.34.0 not yet published ⚠️ still worth confirming — see below

Correctness check

  • Verified the flags the chart emits actually exist and are inherited by snapshot k8s: addAutoEnvironmentFlags registers --auto-environment, --environment-description, --include-scaling, --exclude-scaling as persistent flags on the snapshot group (cmd/kosli/snapshotAutoEnvironment.go:37-40). ✅
  • The tri-state logic emits at most one of --include-scaling / --exclude-scaling, so it never trips the CLI's mutual-exclusion guard (only one of --include-scaling, --exclude-scaling is allowed, snapshotAutoEnvironment.go:63). ✅
  • null default for includeScaling correctly falls through kindIs "bool" → no flag emitted → server default. ✅
  • The whitespace cleanup ({{- chomping) is a nice tidy-up and renders a tighter args: block.
  • values.yaml, README.md, and template are consistent; docstrings clearly note the "shared by all environments / existing environments never modified" caveats. 👍

⚠️ One thing to confirm before merge/publish (release sequencing)

Chart.yaml pins appVersion: "v2.34.0", and the image tag defaults to .Chart.AppVersion (cronjob.yaml:60). The latest published CLI tag is v2.33.3v2.34.0 is not tagged yet. The auto-environment feature is on main (confirmed above), so v2.34.0 is the right pin provided that's the release carrying it.

Action: don't publish chart 2.6.0 until CLI v2.34.0 is actually released, otherwise a default helm install will ImagePullBackOff on a non-existent image tag. Also note the PR description still says "bumped CLI to 2.33.3" while the chart pins v2.34.0 — worth reconciling the description so reviewers aren't confused.

Nit (optional)

  • environmentDescription is a single shared value across all auto-created environments — this is documented, so it's fine as a deliberate limitation. If per-environment descriptions are ever needed, that'd be a follow-up.

Summary

Clean, well-documented follow-up to #1028. All earlier review feedback is incorporated and the emitted CLI flags are valid. The only remaining item is the release-ordering of the chart vs. CLI v2.34.0 — a process check, not a code issue. Looks good to me. ✅

Comment thread charts/k8s-reporter/templates/cronjob.yaml Outdated
@mbevc1
mbevc1 force-pushed the 20260724_helm_auto_env branch from 0b94b2d to 7513d38 Compare July 24, 2026 17:14
Comment thread charts/k8s-reporter/templates/cronjob.yaml Outdated
mbevc1 added 7 commits July 24, 2026 18:29
Expose the CLI's `--auto-environment` flag and its companion flags
(`--environment-description`, `--include-scaling`, `--exclude-scaling`)
as `reporterConfig.*` values on the k8s-reporter chart, so the reporter
CronJob can auto-create Kosli environments (type K8S) on first run
without editing the chart.

Chart version bumped to 2.6.0 and the values
table in README regenerated.
Make reporterConfig.includeScaling nullable (default `null`) so it maps to
all three CLI states: unset omits both scaling flags (Kosli server
default), true passes `--include-scaling`, and false passes
`--exclude-scaling`. The template uses `kindIs "invalid"` to tell an unset
value apart from an explicit false, so autoEnvironment no longer forces a
scaling flag. Also whitespace-trims the auto-environment arg block so it
renders cleanly.
Clarify in the value help text that environmentDescription and
includeScaling apply to every environment auto-created in a single run,
that a single description is shared by all of them, and that existing
environments are never modified (the CLI ignores these flags for envs
that already exist).

Also normalise the args block whitespace: the dryRun and httpProxy
conditionals now use {{- trim markers like the auto-environment block,
so the rendered manifest has no stray blank lines.
Guard the scaling flag with `kindIs "bool"` instead of
`not (kindIs "invalid" ...)`. A string value (e.g. from
`--set-string reporterConfig.includeScaling=false` or a quoted
`includeScaling: "false"`) is Go-template truthy whenever non-empty,
which previously emitted --include-scaling regardless of the intended
value. Requiring a genuine boolean means any non-bool falls back to the
safe "unset -> server default" path.
@mbevc1
mbevc1 force-pushed the 20260724_helm_auto_env branch from 64be161 to c058258 Compare July 24, 2026 17:30
Comment thread charts/k8s-reporter/Chart.yaml
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request helm

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant