mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-08 21:14:56 +09:00
Aleksei Sviridkin
d956536461
<!-- Thank you for making a contribution! Here are some tips for you:
- Use Conventional Commits for the PR title: `type(scope): description`
- Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore
- Scopes are not an exhaustive list — pick the most specific scope for
the change and extend the list when a genuinely new area appears.
Examples:
- System components: dashboard, platform, operator, cilium, kube-ovn,
linstor, fluxcd, cluster-api
- Managed apps: postgres, mariadb, redis, kafka, clickhouse,
virtual-machine, kubernetes
- Development and maintenance: api, hack, tests, ci, docs, maintenance
- Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or
add a `BREAKING CHANGE:` footer
- If it's a work in progress, consider creating this PR as a draft.
- Don't hesistate to ask for opinion and review in the community chats,
even if it's still a draft.
- Add the label `backport` if it's a bugfix that needs to be backported
to a previous version.
-->
## What this PR does
`helm lint packages/apps/tenant` fails on main with three errors. This
removes two of the three; the branch name notwithstanding, lint still
exits 1 afterwards on the third, which is a repository-wide convention
question rather than a defect in this chart.
**The chart cannot be rendered outside a cluster at all.**
`templates/keycloakgroups.yaml` reads `index .Values._cluster
"oidc-enabled"`, and `_cluster` is injected by the platform, so it is
absent whenever the chart is rendered on its own. `index` on a nil map
is a hard error, not an empty result, so both `helm lint` and a plain
`helm template` die on line 1.
The idiom the neighbouring charts use for the same lookup does not
rescue this on its own, which is worth stating because it looks exactly
like a guard. `(index .Values._cluster "x") | default "y"` still fails:
`index` is evaluated first and errors on the nil map, so `default` never
receives a result to substitute; it only ever covered a key missing from
a map that exists.
The spelling alone does not decide whether a chart breaks, though.
`packages/system/cozystack-basics` uses it and lints clean, because line
1 of its `values.yaml` is `_cluster: {}`, which makes the map non-nil
before `index` runs. A chart is broken only when it neither declares the
empty map nor guards at the use site, and a lint sweep across the
catalog found 27 in that state. Guarding at the use site is the choice
here because it makes the template correct whatever `values.yaml`
contains, and because `_cluster` is platform-internal while this chart's
`values.yaml`, schema and README are generated together from
`api/apps/v1alpha1/tenant/types.go` as user-facing output. The guard has
to sit on the map rather than on the value it returns:
```
{{- $oidcEnabled := index (.Values._cluster | default dict) "oidc-enabled" }}
```
**This is behaviour-preserving, and that is checked rather than
asserted.** With `_cluster` present, `| default dict` returns it
untouched and the lookup is what it was; with it absent the lookup is
nil, the `eq` below is false, and the block stays unrendered, which is
what an unconfigured tenant should produce. Rendering before and after
gives byte-identical output:
| values | before | after |
| --- | --- | --- |
| `_cluster.oidc-enabled: "true"` | 4 KeycloakRealmGroups |
byte-identical |
| `_cluster.oidc-enabled: "false"` | no groups | byte-identical |
| no values at all | hard error | renders, no groups |
Worth recording how that comparison has to be run, because the obvious
way proves nothing. The KeycloakRealmGroup block sits behind
`.Capabilities.APIVersions.Has "v1.edp.epam.com/v1"`, which is false
under a plain `helm template`, so a first pass rendered the same output
in *both* the enabled and disabled cases, and a diff of those two would
have compared one empty result against another. The renders above pass
`--api-versions v1.edp.epam.com/v1` so the guarded branch actually
executes. Anyone proving neutrality on a capability-gated template needs
that flag or the proof is vacuous.
**`Chart.yaml` does not declare `cozy-lib`**, which is present as a
symlink under `charts/`, so lint reports missing chart metadata. It is
declared the way `packages/apps/qdrant` does it, that being the one
chart in the tree which declares it and does not carry this error.
Rendering is unchanged by the declaration, checked the same way as
above.
Two things that could plausibly have broken and did not, checked rather
than assumed: rendering from a packaged `helm package` artifact is
byte-identical, and the `charts/cozy-lib` symlink is still expanded into
the tgz with the dependency declared. And neither `hack/update-crd.sh`
nor the `fix-charts` target in `packages/apps/Makefile` reads the
`dependencies:` block, so nothing needs regenerating.
**The icon error is left in place on purpose, and the relative path is
load-bearing.** `helm lint` wants a URL with a scheme, and
`/logos/tenant.svg` has none. But `hack/update-crd.sh` consumes exactly
this form: it rewrites a leading slash to `./`, then reads that file off
disk and base64-encodes it into `.spec.dashboard.icon` of the generated
ApplicationDefinition. `packages/apps/tenant/logos/tenant.svg` is that
file. Give the chart a URL with a scheme and the generator's `[[ ! -f
"$ICON_PATH" ]]` check no longer finds anything to encode. So the
relative form is a requirement of the pipeline rather than a stylistic
convention, all 31 icon-carrying charts use it, and satisfying helm lint
here means changing the generator, not the charts. That is a
repository-wide decision and not this PR's.
`tests/keycloakgroups_oidc_test.yaml` pins the gate in all three
directions: four realm groups when `oidc-enabled` is `"true"`, none when
it is `"false"`, and none when the platform injected no `_cluster` at
all. The last case is the one the guard exists for, and it needs no
mocking, since `_cluster` is absent from `values.yaml` and "not
injected" is just the chart's default state. Against the unguarded
template that suite fails with `index of untyped nil`. The chart's other
seven suites pass identically with and without the fix, which is
precisely why the change needed its own.
Two limits of the new suite, stated rather than left to be found. It
pins how many documents render, not which: renaming the four groups or
breaking a `realmRef` would pass. And the template's second gate,
`.Capabilities.APIVersions.Has "v1.edp.epam.com/v1"`, is enabled for the
whole suite, so no case covers its absence. Both are worth closing on
the next touch of this file; neither weakens what the suite does pin,
which is that the chart renders at all when the platform injected
nothing.
A trap for anyone reproducing this by hand: the gate compares against
the string `"true"`, so `--set _cluster.oidc-enabled=true` passes a YAML
boolean and renders nothing, while `--set-string` renders the four
groups. The platform writes a genuine string, and it survives the
round-trip through a Secret because `toYaml` quotes it. Note also that
this chart's helpers reject a release name that does not begin with
`tenant-`, which will silently render nothing for either form.
One note in case a future change teaches the linter to inject the
platform's values, which would hide this class of error without fixing
it: the guard is still worth having, because it is what lets `helm
template packages/apps/tenant` succeed with no values file at all. A
chart that renders standalone can be inspected, diffed and unit-tested
without reproducing the platform around it.
### Screenshots
<!-- REQUIRED for UI changes: attach screenshots or screen recordings
demonstrating
the visual impact of your changes. PRs with UI changes without
screenshots will not be merged. -->
### Downstream repositories
<!--
Cozystack is upstream for repositories that are NOT kept in sync with it
automatically, and nothing in CI compares the two sides, so a change
here can break them silently.
Which change forces what, and which file to touch, is the trigger map in
docs/agents/contributing.md under "Downstream Repositories". Walk it
before you tick anything.
IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM
TO FILL IN:
1. Walk the trigger map against the actual diff of this PR, file by
file. Do not decide from the PR title or from your memory of what you
changed.
2. For every repository you tick, open the follow-up PR there and link
it on the same line. A ticked box with no link claims work that does not
exist, which is worse than an empty box. Search that repository for an
open PR or issue covering it first, and link that instead of filing a
duplicate.
3. When the follow-up is out of scope, or needs a decision that is not
yours to make, open an issue in that repository instead, link that, and
say so in the PR body. Do not push a speculative PR into another
repository just to fill a line here.
4. Do not tick "No downstream repository is affected" to make the
checklist go away. If you are unsure, leave every box empty and say so
in the PR body, so a human decides.
-->
- [x] No downstream repository is affected by this change
- [ ] [cozystack/website](https://github.com/cozystack/website) -
follow-up:
- [ ]
[cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack)
- follow-up:
- [ ]
[cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack)
- follow-up:
- [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up:
- [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up:
- [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) -
follow-up:
- [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) -
follow-up:
- [ ]
[cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server)
- follow-up:
- [ ]
[cozystack/external-apps-example](https://github.com/cozystack/external-apps-example)
- follow-up:
- [ ] [cozystack/examples](https://github.com/cozystack/examples) -
follow-up:
### Release note
<!-- Write a release note:
- Explain what has changed internally and for users.
- Start with the same `type(scope):` prefix as in the PR title
- Follow the guidelines at
https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md.
-->
```release-note
fix(tenant): the tenant chart now renders without the values the platform injects, so `helm template` works on it standalone, and its `cozy-lib` dependency is declared in Chart.yaml; this clears two of the three `helm lint` errors it reported, the remaining one being the repository-wide relative icon URL
```
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
- **New Features**
- Added support for using the shared Cozy library in tenant deployments.
- **Bug Fixes**
- Improved tenant configuration rendering when cluster settings are
unavailable.
- OIDC-related Keycloak groups now render reliably when OIDC is enabled
and required platform capabilities are present.
- Preserved expected behavior when OIDC is disabled or configuration is
omitted.
- **Tests**
- Added coverage for enabled, disabled, and missing cluster
configuration scenarios.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
How to test packages local
cd packages/core/installer
make image-cozystack REGISTRY=YOUR_CUSTOM_REGISTRY
make apply
kubectl delete po -l app=source-controller -n cozy-fluxcd