Allow users to apply labels and annotations to internal resources#4400
Allow users to apply labels and annotations to internal resources#4400nikola-jokic wants to merge 5 commits intomasterfrom
Conversation
|
Hello! Thank you for your contribution. Please review our contribution guidelines to understand the project's testing and code conventions. |
There was a problem hiding this comment.
Pull request overview
Adds support for specifying per-internal-resource labels/annotations via new ResourceMeta fields on AutoscalingRunnerSet / AutoscalingListener / EphemeralRunner(Set) APIs, and wires that metadata into the controller’s resource builder (with accompanying CRD updates).
Changes:
- Introduce
ResourceMetaAPI type and new spec fields to carry labels/annotations for internal resources. - Apply the new metadata in
ResourceBuilderwhen creating listener/runner internal resources. - Regenerate/update CRDs (repo config + Helm chart packaged CRDs) and expand unit tests for propagation.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| controllers/actions.github.com/resourcebuilder.go | Applies metadata to listener/runner internal resources; adds new merge helpers |
| controllers/actions.github.com/resourcebuilder_test.go | Expands propagation test to cover new metadata fields |
| apis/actions.github.com/v1alpha1/common.go | Adds ResourceMeta type |
| apis/actions.github.com/v1alpha1/autoscalingrunnerset_types.go | Adds new metadata fields to AutoscalingRunnerSetSpec |
| apis/actions.github.com/v1alpha1/autoscalinglistener_types.go | Adds new metadata fields to AutoscalingListenerSpec |
| apis/actions.github.com/v1alpha1/ephemeralrunnerset_types.go | Adds EphemeralRunner metadata field |
| apis/actions.github.com/v1alpha1/ephemeralrunner_types.go | Adds EphemeralRunner JIT secret metadata field |
| apis/actions.github.com/v1alpha1/zz_generated.deepcopy.go | Updates deepcopy generation for new fields/types |
| config/crd/bases/actions.github.com_autoscalingrunnersets.yaml | CRD schema update for new fields |
| config/crd/bases/actions.github.com_autoscalinglisteners.yaml | CRD schema update for new fields |
| config/crd/bases/actions.github.com_ephemeralrunnersets.yaml | CRD schema update for new fields |
| config/crd/bases/actions.github.com_ephemeralrunners.yaml | CRD schema update for new fields |
| charts/gha-runner-scale-set-controller/crds/actions.github.com_autoscalingrunnersets.yaml | Packaged chart CRD schema update |
| charts/gha-runner-scale-set-controller/crds/actions.github.com_autoscalinglisteners.yaml | Packaged chart CRD schema update |
| charts/gha-runner-scale-set-controller/crds/actions.github.com_ephemeralrunnersets.yaml | Packaged chart CRD schema update |
| charts/gha-runner-scale-set-controller/crds/actions.github.com_ephemeralrunners.yaml | Packaged chart CRD schema update |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| required: | ||
| - annotations | ||
| - labels | ||
| type: object |
There was a problem hiding this comment.
This ResourceMeta schema requires both annotations and labels. If the intent is to allow users to set only one of them, the schema should not list them under required: (adjust ResourceMeta fields to be optional and regenerate CRDs).
| required: | ||
| - annotations | ||
| - labels | ||
| type: object |
There was a problem hiding this comment.
This ResourceMeta schema in the chart CRD requires both annotations and labels. If only one should be needed, adjust the Go type to use optional fields and regenerate the CRDs included in the chart to drop these required: entries.
| required: | ||
| - annotations | ||
| - labels | ||
| type: object |
There was a problem hiding this comment.
This chart CRD requires both annotations and labels for ephemeralRunnerConfigSecretMetadata. If the intent is to allow setting just annotations (common for IAM role bindings), remove these from required: by making the Go fields optional and regenerating the CRDs.
| ServiceAccountMetadata: autoscalingRunnerSet.Spec.ListenerServiceAccountMetadata, | ||
| RoleMetadata: autoscalingRunnerSet.Spec.ListenerRoleMetadata, | ||
| RoleBindingMetadata: autoscalingRunnerSet.Spec.ListenerRoleMetadata, | ||
| ListenerConfigSecretMetadata: autoscalingRunnerSet.Spec.ListenerConfigSecretMetadata, |
There was a problem hiding this comment.
RoleBindingMetadata is being populated from ListenerRoleMetadata, so any role-binding-specific labels/annotations provided via ListenerRoleBindingMetadata are ignored. This also makes the listener RoleBinding pick up the Role metadata instead.
| var annotations map[string]string | ||
|
|
||
| for autoscalingListener.Spec.RoleBindingMetadata != nil { | ||
| labels = b.filterAndMergeLabels(autoscalingListener.Spec.RoleBindingMetadata.Labels, labels) | ||
| annotations = autoscalingListener.Spec.RoleBindingMetadata.Annotations | ||
| } |
There was a problem hiding this comment.
This uses a for autoscalingListener.Spec.RoleBindingMetadata != nil { ... } loop, which will never terminate when metadata is non-nil and will hang the controller/tests. This should be an if (or a loop that breaks) to apply metadata once.
| required: | ||
| - annotations | ||
| - labels |
There was a problem hiding this comment.
These ResourceMeta schema blocks require both annotations and labels, which forces users to set empty maps when they only need one. Consider making ResourceMeta fields optional in the Go API and regenerating the vendored chart CRDs so required: does not include them.
| required: | ||
| - annotations | ||
| - labels | ||
| type: object |
There was a problem hiding this comment.
ResourceMeta is defined here with both annotations and labels in required:, which forces users to provide an empty map for the unused field. Consider making these properties optional by updating the API type and regenerating this chart CRD.
| func (b *ResourceBuilder) filterAndMergeLabels(base, overwrite map[string]string) map[string]string { | ||
| if base == nil && overwrite == nil { | ||
| return nil | ||
| } | ||
| if len(overwrite) == 0 { | ||
| return maps.Clone(base) | ||
| } |
There was a problem hiding this comment.
filterAndMergeLabels returns maps.Clone(base) when overwrite is empty, which skips the prefix filtering logic entirely. Call sites like filterAndMergeLabels(customLabels, nil) will therefore allow excluded/reserved label prefixes to pass through unfiltered; consider always running the filter even when not merging.
| // +optional | ||
| AutoscalingListenerMetadata *ResourceMeta `json:"autoscalingListener,omitempty"` | ||
|
|
||
| // +optional | ||
| ListenerMetrics *MetricsConfig `json:"listenerMetrics,omitempty"` | ||
|
|
||
| // +optional | ||
| ListenerTemplate *corev1.PodTemplateSpec `json:"listenerTemplate,omitempty"` | ||
|
|
||
| // +optional | ||
| ListenerServiceAccountMetadata *ResourceMeta `json:"listener_service_account_metadata,omitempty"` | ||
|
|
||
| // +optional | ||
| ListenerRoleMetadata *ResourceMeta `json:"listener_role_metadata,omitempty"` | ||
|
|
||
| // +optional | ||
| ListenerRoleBindingMetadata *ResourceMeta `json:"listener_role_binding_metadata,omitempty"` | ||
|
|
||
| // +optional | ||
| ListenerConfigSecretMetadata *ResourceMeta `json:"listener_config_secret_metadata,omitempty"` | ||
|
|
||
| // +optional | ||
| EphemeralRunnerSetMetadata *ResourceMeta `json:"ephemeralRunnerSetMetadata,omitempty"` | ||
|
|
||
| // +optional | ||
| EphemeralRunnerMetadata *ResourceMeta `json:"ephemeralRunnerMetadata,omitempty"` | ||
|
|
||
| // +optional | ||
| EphemeralRunnerConfigSecretMetadata *ResourceMeta `json:"ephemeralRunnerConfigSecretMetadata,omitempty"` | ||
|
|
There was a problem hiding this comment.
The new JSON field names mix lowerCamelCase (e.g. autoscalingListener, ephemeralRunnerSetMetadata) with snake_case (e.g. listener_service_account_metadata). This is inconsistent with the existing API fields in this spec (which are lowerCamelCase) and is likely to be surprising for CR authors/Helm values; consider renaming these to lowerCamelCase and regenerating CRDs accordingly.
| required: | ||
| - annotations | ||
| - labels |
There was a problem hiding this comment.
These ResourceMeta schema blocks mark both annotations and labels as required. That forces users to provide (at least) an empty map for the unused field, which makes it harder to set only annotations (the reported use case). Consider making labels/annotations optional in the Go type (omitempty/+optional) and regenerating CRDs so these required: entries are removed.
Fixes #4293