feat(helm): add operational labels to GithubOrganization - #248
onuryilmaz wants to merge 6 commits into
Conversation
Adds four labels to every GithubOrganization CR rendered by the Helm chart, enabling Permission Manager to look up org CRs via label selectors and read config without parsing spec fields: - repo-guard.cloudoperators.dev/github-instance: full hostname - repo-guard.cloudoperators.dev/github-instance-key: short key (defaults to first segment of the github field, e.g. "enterprise" from "enterprise.github.com"); overridable via githubInstanceKey - repo-guard.cloudoperators.dev/default-ldap-provider: LDAP provider PM writes into GithubTeam CRs it creates for this org - repo-guard.cloudoperators.dev/admin-permission: "admin" or "admin-ondemand" (defaults to "admin") Closes #246 Signed-off-by: Onur Yilmaz <onur.yilmaz@sap.com>
There was a problem hiding this comment.
🟡 Changes recommended
Two moderate issues affect label lookup and Kubernetes validity; the public label documentation is also incomplete.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds four Permission Manager operational labels to Helm-rendered GithubOrganization resources and documents their configurable values.
Changes:
- Adds instance, instance-key, LDAP-provider, and admin-permission labels.
- Documents label overrides and defaults in
values.yaml.
File summaries
| File | Summary | Review findings |
|---|---|---|
charts/repo-guard/values.yaml |
Documents operational-label configuration values. | No findings. |
charts/repo-guard/templates/githuborganization.yaml |
Renders the four operational labels. | Moderate (3 votes): $org.github is a CR reference/resource key rather than necessarily the hostname, so hostname-based lookup can fail; derive labels from the matching instance URL or update the values contract. Moderate (1 vote): Full hostnames may exceed Kubernetes’s 63-character label-value limit and cause manifest rejection. Nit (1 vote): Add all four labels and defaults to docs/operations/labels.md. |
Review details
Suppressed comments (3)
charts/repo-guard/templates/githuborganization.yaml:24
- The label advertises a key that Permission Manager uses to construct
<instance-key>--...names, but the organization and team templates still build those names from$org.github. Withgithub: enterprise.github.com, the label defaults toenterprisewhile rendered names remainenterprise.github.com--...; withgithubInstanceKeyoverridden, the mismatch is guaranteed. Use the same naming prefix for the rendered resources or expose the actual prefix to Permission Manager before relying on this label.
repo-guard.cloudoperators.dev/github-instance-key: "{{ $org.githubInstanceKey | default ($org.github | splitList "." | first) }}"
charts/repo-guard/templates/githuborganization.yaml:23
- A full hostname is not an unconstrained Kubernetes label value: label values are limited to 63 characters. Since
$org.githubcan be a valid longer hostname/resource name, this can make the entireGithubOrganizationmanifest rejected by the API. Restrict the accepted hostname length or define a label-safe lookup representation while preserving the full hostname through another field.
repo-guard.cloudoperators.dev/github-instance: "{{ $org.github }}"
charts/repo-guard/templates/githuborganization.yaml:26
docs/operations/labels.mdis the repository's reference table forGithubOrganizationlabels, but it does not document any of these four newly public labels or their defaults. Add entries for the lookup key, instance key, LDAP provider, and admin-permission labels so the chart/Permission Manager contract is discoverable and consistent with the existing label reference.
repo-guard.cloudoperators.dev/github-instance: "{{ $org.github }}"
repo-guard.cloudoperators.dev/github-instance-key: "{{ $org.githubInstanceKey | default ($org.github | splitList "." | first) }}"
repo-guard.cloudoperators.dev/default-ldap-provider: "{{ $org.defaultLdapProvider | default "" }}"
repo-guard.cloudoperators.dev/admin-permission: "{{ $org.adminPermission | default "admin" }}"
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- github-instance-key now defaults to the full spec.github value (e.g. "enterprise.github.com") instead of just the first hostname segment. This matches the actual CR naming prefix used by the Helm chart (<github>--<org>), so Permission Manager constructs correct names without needing an explicit override. - Update values.yaml comment to reflect the corrected default. - Document the four new operational labels in docs/operations/labels.md, including the 63-char Kubernetes label value constraint on github-instance. Signed-off-by: Onur Yilmaz <onur.yilmaz@sap.com>
|
Addressed the Copilot review findings in the follow-up commit:
The original default used 63-char label value limit The Docs Added all four operational labels to the |
There was a problem hiding this comment.
🟡 Changes recommended
Label derivation and resource naming are inconsistent, and hostname-based labels lack length validation.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (5)
charts/repo-guard/templates/githuborganization.yaml:23
$org.githubis theGithubresource key, not the GitHub hostname: the chart examples usegithub: comwhile the hostname is configured asgithubs[].webURL(seecharts/repo-guard/templates/github.yaml:8-10andvalues.yaml:93-110). This label will therefore rendercom, so Permission Manager cannot select the org from a hostname such asenterprise.github.com; derive it from the associatedGithubobject'swebURLor introduce an explicit hostname value, and update the label documentation accordingly.
repo-guard.cloudoperators.dev/github-instance: "{{ $org.github }}"
charts/repo-guard/templates/githuborganization.yaml:24
- This fallback emits the entire hostname rather than the required first segment: with
github: enterprise.github.com, the label isenterprise.github.cominstead ofenterprise. Permission Manager will then use the wrong instance key when constructing CR names.
repo-guard.cloudoperators.dev/github-instance-key: "{{ $org.githubInstanceKey | default $org.github }}"
charts/repo-guard/templates/githuborganization.yaml:23
- A full hostname can be longer than Kubernetes' 63-character label-value limit, but this template emits it without validation. A valid GitHub Enterprise hostname over that limit will render successfully in Helm and then be rejected by the Kubernetes API, preventing the release from installing. Fail early in the chart or use a label-safe identifier for lookup.
repo-guard.cloudoperators.dev/github-instance: "{{ $org.github }}"
charts/repo-guard/values.yaml:136
- This chart comment documents the opposite default from the PR contract:
githubInstanceKeyis supposed to default to the first segment of a hostname, not the fullgithubvalue. Update it together with the template so users do not configure Permission Manager with the wrong CR-name prefix.
# # githubInstanceKey: defaults to the 'github' field value; used as the prefix in CR names (<instance-key>--<org>--<slug>)
docs/operations/labels.md:31
- The documented default here contradicts the PR requirement and the test plan: it says the full
spec.githubvalue, while the requested default is its first hostname segment (for example,enterprisefromenterprise.github.com). Update this description and default after correcting the template.
| `repo-guard.cloudoperators.dev/github-instance-key` | Key PM uses to construct CR names matching the repo-guard convention (`<instance-key>--<org>--<team-slug>`). Defaults to the full `spec.github` value, which matches how the Helm chart names `GithubOrganization` CRs. Override via `githubInstanceKey` in Helm values. | `spec.github` value |
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
The github-instance label must carry the full GitHub hostname (e.g. enterprise.github.com) so Permission Manager can match it against the instance segment in CCRN URNs. The previous value ($org.github) was the Github CR name key, not the hostname. - Look up the matching githubs[] entry by name and use its webURL as the default; fall back to an explicit githubInstanceHostname override. - github-instance-key keeps $org.github as its default — that is the correct naming prefix used in <instance-key>--<org>--<team-slug> CRs. - Update values.yaml comments and docs/operations/labels.md accordingly. Signed-off-by: Onur Yilmaz <onur.yilmaz@sap.com>
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved naming, label-length validation, and admin-permission validation issues block approval.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
charts/repo-guard/templates/githuborganization.yaml:24
- This default does not match the PR contract/test plan: for
github: enterprise.github.comit rendersenterprise.github.com, not the first segmententerprise. More importantly, the chart and controller currently use$org.github/spec.githubfor theGithubOrganizationandGithubTeamnames, so using a short key (or an override different from$org.github) makes Permission Manager's<instance-key>--...names diverge from the resources this chart creates. Please choose one canonical naming key and apply it consistently across the label, chart names, and controller/consumer contract, or keep the full hostname as the documented key.
{{- if not $githubInstance -}}
charts/repo-guard/templates/githuborganization.yaml:23
$org.githubis the name of the referencedGithubCR, not its hostname (GithubOrganization.spec.githubis documented as a resource name; the host is in theGithubobject'sspec.webURL). With the normalcom/https://github.comsetup this rendersgithub-instance: "com", so a selector forgithub.comcannot find the organization. Resolve the referenced Github's hostname (or add an explicit hostname value) before writing this label.
{{- $githubInstance := $org.githubInstanceHostname -}}
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
The Github CR's webURL field contains a full URL including scheme (e.g. https://github.com). The PM CCRN instance segment is the hostname only (e.g. github.com). Strip the https:// / http:// prefix so the label value matches what PM extracts from CCRN URNs. Signed-off-by: Onur Yilmaz <onur.yilmaz@sap.com>
Fail helm template/install early if adminPermission is set to anything other than "admin" or "admin-ondemand", preventing a typo from reaching Permission Manager as an invalid ADMIN role mapping. Signed-off-by: Onur Yilmaz <onur.yilmaz@sap.com>
There was a problem hiding this comment.
🟡 Changes recommended
The template has a critical missing-webURL failure path and a moderate resource-naming mismatch.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
charts/repo-guard/templates/githuborganization.yaml:31
- The value derived from
webURLis copied into a Kubernetes label without validating or normalizing it. A valid configured URL such ashttps://ghe.example.com/or a hostname longer than 63 characters produces a value with/or an overlong label, so the renderedGithubOrganizationis rejected by the API and Permission Manager cannot use the lookup. Validate the derived/override value against Kubernetes label-value constraints (or parse the URL to a hostname and fail clearly when it cannot fit).
repo-guard.cloudoperators.dev/github-instance: "{{ $githubInstance }}"
charts/repo-guard/templates/githuborganization.yaml:33
- Both
githubInstanceKeyanddefaultLdapProvideroverrides are emitted verbatim as label values, but Kubernetes label values are limited to 63 characters and have character/start-end restrictions. A valid longer resource/provider name therefore makes Helm render aGithubOrganizationthat the API rejects. Add render-time validation for these label values and document the constraints; do not truncate them because Permission Manager needs the exact key/provider name.
repo-guard.cloudoperators.dev/github-instance-key: "{{ $org.githubInstanceKey | default $org.github }}"
repo-guard.cloudoperators.dev/default-ldap-provider: "{{ $org.defaultLdapProvider | default "" }}"
charts/repo-guard/templates/githuborganization.yaml:32
githubInstanceKeyonly changes this label, but the chart still namesGithubOrganization,GithubTeam, andGithubTeamRepositoryresources with$org.github. With an override such asgithubInstanceKey: enterpriseandgithub: enterprise.github.com, Permission Manager will constructenterprise--...names that Helm never rendered. Apply the effective key to the generated resource names (while retaining the original value inspec.github), or remove/limit this override so the label cannot diverge from the naming prefix.
repo-guard.cloudoperators.dev/github-instance-key: "{{ $org.githubInstanceKey | default $org.github }}"
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
The mock GitHub server URL (e.g. http://github-mock.svc:8080) contains a port number, and Kubernetes rejects label values containing colons. Also default webURL to empty string before trimming to avoid nil dereference when webURL is omitted from a githubs[] entry. Use splitList ":" | first to strip the port after scheme removal, which handles both http://host:port and https://host forms correctly. Signed-off-by: Onur Yilmaz <onur.yilmaz@sap.com>
Summary
Adds four operational labels to every
GithubOrganizationCR rendered by the Helm chart, so Permission Manager can look up org CRs via label selectors and read config without parsingspecfields.repo-guard.cloudoperators.dev/github-instance— full hostname; PM uses as a label selector to find the org CR from a CCRN hostnamerepo-guard.cloudoperators.dev/github-instance-key— short key (defaults to first segment of thegithubfield, e.g.enterprisefromenterprise.github.com); overridable viagithubInstanceKeyin valuesrepo-guard.cloudoperators.dev/default-ldap-provider— LDAP provider PM writes intospec.externalMemberProvider.ldap.provideronGithubTeamCRs it creates for this orgrepo-guard.cloudoperators.dev/admin-permission—"admin"or"admin-ondemand"; PM maps theADMINrole to this value (defaults to"admin")No CRD or Go changes required — these are purely
ObjectMetalabels.Closes #246
Test plan
helm templaterenders all four labels with correct defaults (github-instance-key= first hostname segment,admin-permission="admin",default-ldap-provider="")githubInstanceKey,defaultLdapProvider,adminPermission) render correctlyhelm lint charts/repo-guardpasses