[HYPERFLEET-479]: Create Helm chart for adapter-pull-secret deployment#8
[HYPERFLEET-479]: Create Helm chart for adapter-pull-secret deployment#8tzhou5 wants to merge 4 commits intoopenshift-hyperfleet:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughThis PR refactors the pull-secret Helm chart into an Adapter Framework: adds a Deployment-based adapter, broker config and broker ConfigMap, ConfigMaps embedding adapter and job templates, RBAC, redesigned serviceAccount handling, updated helpers, and a status-reporter sidecar Job template. values.yaml is reorganized into layered/global keys (image, serviceAccount, rbac, broker, hyperfleetApi, pullSecretAdapter, scheduling, etc.). README, NOTES, and Chart metadata updated; old job.yaml removed and .gitignore entries made root-relative. Sequence Diagram(s)sequenceDiagram
participant PubSub as Pub/Sub
participant Adapter as Pull‑Secret Adapter (Deployment)
participant K8sAPI as Kubernetes API
participant Job as Pull‑Secret Job (created)
participant Status as Status Reporter (sidecar)
participant HyperFleet as HyperFleet API
PubSub->>Adapter: message/event (pull secret request)
Adapter->>K8sAPI: create Job using job-template (ConfigMap)
K8sAPI-->>Job: schedule/run Job (pull-secret + status-reporter)
Job->>Status: write results to /results (shared volume)
Status->>HyperFleet: report job status/results
Status->>K8sAPI: optionally update Job status / logs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@charts/pull-secret/templates/_helpers.tpl`:
- Around line 58-63: The helper pull-secret.serviceAccountName currently returns
"default" when serviceAccount.create is false, which risks granting cluster-wide
permissions to the namespace default SA when rbac.create is true; change the
template to require an explicit .Values.serviceAccount.name when
.Values.serviceAccount.create is false and .Values.rbac.create is true (use
Helm's required function or equivalent) so deployment fails unless the user
provides a name (they may still explicitly set it to "default" if they intend
to); keep the existing behavior for the case where serviceAccount.create is
true.
🧹 Nitpick comments (3)
charts/pull-secret/README.md (2)
62-68: Consider documenting the helm-git plugin requirement.The
git+https://...@path?ref=branchrepository format requires the helm-git plugin. Users integrating this as an umbrella chart dependency should be aware they need this plugin installed.
85-100: Configuration table is incomplete.The table omits several parameters defined in
values.yaml:
job.backoffLimit,job.ttlSecondsAfterFinished,job.restartPolicyhyperfleetApi.baseUrl,hyperfleetApi.versionimage.pullPolicy,imagePullSecretsnodeSelector,tolerations,affinityConsider adding these for completeness, or add a note that only commonly-customized parameters are documented here.
charts/pull-secret/values.yaml (1)
25-29: Consider using a pinned image tag for production deployments.Using
latestas the default tag is acceptable for development, but for production deployments, a specific version tag (e.g., semantic version) provides better reproducibility and rollback capability. The comment or documentation could note this recommendation.
tzhou5
force-pushed
the
add-helm-chart
branch
from
a448696 to
86cd68b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@charts/pull-secret/templates/rbac.yaml`:
- Around line 32-35: Update the helper that renders
pull-secret.serviceAccountName in _helpers.tpl to guard against implicit
defaulting: when .Values.rbac.create is true and .Values.serviceAccount.create
is false, require an explicit .Values.serviceAccount.name (and fail/render a
clear error or empty/omit the subject) instead of returning the namespace
default; change the helper logic to check .Values.rbac.create,
.Values.serviceAccount.create, and .Values.serviceAccount.name and return the
explicit name or produce a template error message so ClusterRoleBinding cannot
bind the cluster role to the namespace default ServiceAccount.
In `@charts/pull-secret/values.yaml`:
- Around line 70-93: Add validation so Helm fails early for the required fields:
either update templates that consume .Values.gcp.projectId, .Values.cluster.id
and .Values.pullSecret.data to use the required function (e.g., replace
references with {{ .Values.gcp.projectId | required "gcp.projectId is required"
}}, {{ .Values.cluster.id | required "cluster.id is required" }}, and {{
.Values.pullSecret.data | required "pullSecret.data is required" }}) or create a
charts/pull-secret/values.schema.json declaring gcp.projectId, cluster.id and
pullSecret.data as required properties (with proper JSON schema types) so helm
install/upgrade validates them at chart install time.
♻️ Duplicate comments (1)
charts/pull-secret/templates/_helpers.tpl (1)
58-64: Require explicit ServiceAccount name when binding ClusterRole.When
serviceAccount.create=falseandrbac.create=true, the helper returns"default", causing the ClusterRoleBinding to grant cluster-wide secret permissions to the namespace's default ServiceAccount. This is a privilege escalation risk.Require an explicit
serviceAccount.namein this scenario.Suggested fix
{{- define "pull-secret.serviceAccountName" -}} {{- if .Values.serviceAccount.create }} {{- default (include "pull-secret.fullname" .) .Values.serviceAccount.name }} +{{- else if .Values.rbac.create }} +{{- required "serviceAccount.name must be set when serviceAccount.create=false and rbac.create=true" .Values.serviceAccount.name }} {{- else }} {{- default "default" .Values.serviceAccount.name }} {{- end }} {{- end }}
🧹 Nitpick comments (4)
charts/pull-secret/templates/NOTES.txt (1)
35-35: Handle missing ttlSecondsAfterFinished gracefully.If
.Values.job.ttlSecondsAfterFinishedis not set, this line will render as "cleaned up seconds after completion," which could confuse users.♻️ Proposed fix to handle missing value
-The job will be automatically cleaned up {{ .Values.job.ttlSecondsAfterFinished }} seconds after completion. +{{- if .Values.job.ttlSecondsAfterFinished }} +The job will be automatically cleaned up {{ .Values.job.ttlSecondsAfterFinished }} seconds after completion. +{{- else }} +The job does not have TTL configured and will not be automatically cleaned up. +{{- end }}charts/pull-secret/README.md (1)
134-135: Clarify required field and auto-generation logic.Consider enhancing the documentation:
pullSecret.name: Clarify the auto-generation format ishyperfleet-{cluster.id}-pull-secretpullSecret.data: Mark explicitly as required since without it the job won't have credentials to storeSuggested documentation update
-| `pullSecret.name` | Secret name in GCP Secret Manager | Auto-generated | -| `pullSecret.data` | Pull secret JSON data (required) | `""` | +| `pullSecret.name` | Secret name in GCP Secret Manager | `hyperfleet-{cluster.id}-pull-secret` | +| `pullSecret.data` | Pull secret JSON data (**required**) | `""` |charts/pull-secret/templates/job.yaml (1)
36-42: Consider validating required environment variables.
GCP_PROJECT_IDandCLUSTER_IDare rendered even if empty, which may cause the job to fail at runtime with unclear errors. Consider using Helm'srequiredfunction to fail fast during template rendering.Suggested validation
env: - name: GCP_PROJECT_ID - value: {{ .Values.gcp.projectId | quote }} + value: {{ required "gcp.projectId is required" .Values.gcp.projectId | quote }} - name: CLUSTER_ID - value: {{ .Values.cluster.id | quote }} + value: {{ required "cluster.id is required" .Values.cluster.id | quote }} - name: SECRET_NAME value: {{ include "pull-secret.secretName" . | quote }}charts/pull-secret/values.yaml (1)
25-31: Avoidlatest+Alwaysfor reproducible rollouts.
Consider pinning the tag and defaulting toIfNotPresentso deployments are deterministic.Suggested values tweak
image: registry: quay.io/openshift-hyperfleet repository: pull-secret - tag: "latest" - pullPolicy: Always + # Prefer a pinned tag; if templates default empty to Chart.AppVersion, keep this empty. + tag: "" + pullPolicy: IfNotPresent
| subjects: | ||
| - kind: ServiceAccount | ||
| name: {{ include "pull-secret.serviceAccountName" . }} | ||
| namespace: {{ .Release.Namespace }} |
There was a problem hiding this comment.
ServiceAccount binding depends on helper logic with potential privilege escalation.
The ClusterRoleBinding references the service account via pull-secret.serviceAccountName. As flagged in a previous review, when serviceAccount.create=false and rbac.create=true, the helper defaults to the namespace's default ServiceAccount, granting it cluster-wide secret permissions unintentionally.
Ensure the helper in _helpers.tpl is updated to require an explicit serviceAccount.name when rbac.create=true but serviceAccount.create=false.
🤖 Prompt for AI Agents
In `@charts/pull-secret/templates/rbac.yaml` around lines 32 - 35, Update the
helper that renders pull-secret.serviceAccountName in _helpers.tpl to guard
against implicit defaulting: when .Values.rbac.create is true and
.Values.serviceAccount.create is false, require an explicit
.Values.serviceAccount.name (and fail/render a clear error or empty/omit the
subject) instead of returning the namespace default; change the helper logic to
check .Values.rbac.create, .Values.serviceAccount.create, and
.Values.serviceAccount.name and return the explicit name or produce a template
error message so ClusterRoleBinding cannot bind the cluster role to the
namespace default ServiceAccount.
charts/pull-secret/values.yaml
Outdated
| gcp: | ||
| # GCP project ID where secrets will be stored | ||
| # IMPORTANT: Replace with your actual GCP project ID | ||
| gcpProjectId: "your-gcp-project" | ||
| # REQUIRED: Must be set before deployment | ||
| projectId: "" | ||
|
|
||
| # Cluster identifier | ||
| # IMPORTANT: Replace with your actual cluster ID | ||
| clusterId: "your-cluster-id" | ||
| # ============================================================================= | ||
| # Cluster Configuration | ||
| # ============================================================================= | ||
| cluster: | ||
| # Cluster identifier - used to name the secret in GCP | ||
| # REQUIRED: Must be set before deployment | ||
| id: "" | ||
|
|
||
| # Secret name in GCP Secret Manager (auto-derived if not provided) | ||
| # This will be auto-generated as: hyperfleet-{clusterId}-pull-secret | ||
| secretName: "hyperfleet-your-cluster-id-pull-secret" | ||
| # ============================================================================= | ||
| # Pull Secret Configuration | ||
| # ============================================================================= | ||
| pullSecret: | ||
| # Secret name in GCP Secret Manager | ||
| # Auto-generated as: hyperfleet-{cluster.id}-pull-secret if empty | ||
| name: "" | ||
| # Pull secret JSON data | ||
| # REQUIRED: Must be set before deployment | ||
| # Example: '{"auths":{"registry.example.com":{"auth":"...","email":"user@example.com"}}}' | ||
| data: "" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Chart structure ==="
find charts/pull-secret -type f | head -20
echo -e "\n=== Check for values.schema.json ==="
find charts/pull-secret -name "values.schema.json" -o -name "values-schema.json"
echo -e "\n=== Check for required() usage in templates ==="
if [ -d "charts/pull-secret/templates" ]; then
rg -n 'required' charts/pull-secret/templates || echo "No 'required' found"
else
echo "templates directory not found"
fi
echo -e "\n=== Check for Values references to required fields ==="
if [ -d "charts/pull-secret/templates" ]; then
rg -n '\.Values\.(gcp\.projectId|cluster\.id|pullSecret\.data)' charts/pull-secret/templates || echo "No specific references found"
else
echo "templates directory not found"
fiRepository: openshift-hyperfleet/adapter-pull-secret
Length of output: 1646
Add validation for required fields using Helm's required function or a values.schema.json file.
Fields marked as required (gcp.projectId, cluster.id, pullSecret.data) currently lack enforcement. While comments and NOTES warnings document the requirement, Helm will render templates with empty values, causing failures downstream instead of at validation time.
Add either:
required()calls in templates (e.g.,{{ .Values.gcp.projectId | required "gcp.projectId is required" }}), or- A
charts/pull-secret/values.schema.jsonwith required field constraints
This ensures validation failures occur at helm install/upgrade time rather than during job execution.
🤖 Prompt for AI Agents
In `@charts/pull-secret/values.yaml` around lines 70 - 93, Add validation so Helm
fails early for the required fields: either update templates that consume
.Values.gcp.projectId, .Values.cluster.id and .Values.pullSecret.data to use the
required function (e.g., replace references with {{ .Values.gcp.projectId |
required "gcp.projectId is required" }}, {{ .Values.cluster.id | required
"cluster.id is required" }}, and {{ .Values.pullSecret.data | required
"pullSecret.data is required" }}) or create a
charts/pull-secret/values.schema.json declaring gcp.projectId, cluster.id and
pullSecret.data as required properties (with proper JSON schema types) so helm
install/upgrade validates them at chart install time.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
charts/pull-secret/README.md (1)
118-140: Config table is missing newly introduced keys.The table doesn’t list several keys added in this PR (e.g.,
imagePullSecrets,podSecurityContext,securityContext,nodeSelector,affinity,tolerations,env,hyperfleetApi.*). Please add them so users can discover the new schema.
🧹 Nitpick comments (1)
charts/pull-secret/README.md (1)
43-46: Document a safer way to passpullSecret.data.Inline
--set pullSecret.data='{"auths":...}'and embedding raw JSON in values can leak secrets to shell history and Git. Consider recommending--set-file(or an external secret) in the docs.Proposed doc tweak
- --set pullSecret.data='{"auths":{...}}' \ + --set-file pullSecret.data=./pull-secret.json \Also applies to: 60-63
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@charts/pull-secret/README.md`:
- Around line 92-98: The README's Umbrella Chart Integration uses a git+
repository URL in the dependencies block (repository:
"git+https://github.com/openshift-hyperfleet/adapter-pull-secret@charts/pull-secret?ref=main")
which requires the helm-git plugin; update the Prerequisites section to document
that requirement by adding an entry for installing the helm-git plugin (include
the install command and recommended version), or instead change the repository
format to a standard Helm repo (OCI or HTTP chart repo) so the git+ URL is not
necessary; reference the dependencies block and the repository field when making
the change.
🧹 Nitpick comments (1)
charts/pull-secret/README.md (1)
142-142: Clarify the default value syntax.The default value
hyperfleet-{cluster.id}-pull-secretuses curly braces which could be ambiguous. Consider using one of these clearer alternatives:
hyperfleet-<cluster.id>-pull-secret(angle brackets indicate placeholder)- Auto-generated:
hyperfleet-CLUSTER_ID-pull-secret- Or add a note:
hyperfleet-{cluster.id}-pull-secret (auto-generated)📝 Suggested clarification
-| `pullSecret.name` | Secret name in GCP Secret Manager | `hyperfleet-{cluster.id}-pull-secret` | +| `pullSecret.name` | Secret name in GCP Secret Manager | `hyperfleet-<cluster.id>-pull-secret` (auto-generated) |
| ```yaml | ||
| dependencies: | ||
| - name: pull-secret | ||
| version: "0.1.0" | ||
| repository: "git+https://github.com/openshift-hyperfleet/adapter-pull-secret@charts/pull-secret?ref=main" | ||
| condition: pull-secret.enabled | ||
| ``` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for helm-git mentions and repository setup documentation
rg -n -C5 'helm-git|git\+https|repository.*git' charts/pull-secret/ --type md
echo "---"
# Also check if there are any prerequisites or setup docs
rg -n -i 'prerequisite|requirement|setup|install' charts/pull-secret/README.md
echo "---"
# Check the full README structure to see all sections
head -100 charts/pull-secret/README.mdRepository: openshift-hyperfleet/adapter-pull-secret
Length of output: 3381
🏁 Script executed:
#!/bin/bash
# Check if this git+ format is used elsewhere in the codebase
git ls-files | xargs rg -l 'git\+https.*\?ref=' 2>/dev/null | head -20Repository: openshift-hyperfleet/adapter-pull-secret
Length of output: 112
🏁 Script executed:
#!/bin/bash
# Look for any Helm documentation or references to special repository types
find . -name "*.md" -type f | xargs rg -l 'helm-git|git.*repository|OCI.*registry' 2>/dev/null | head -10Repository: openshift-hyperfleet/adapter-pull-secret
Length of output: 66
Document the helm-git plugin requirement for Umbrella Chart Integration.
The git+ repository format in the Umbrella Chart Integration section (line 96) requires the helm-git plugin to function. Add this to the Prerequisites section:
4. **helm-git plugin installed** (required for umbrella chart integration with git+ repositories)
```bash
helm plugin install https://github.com/aslafy-z/helm-git --version 0.15.1
Alternatively, use a standard Helm repository format (OCI registry or HTTP chart repository URL) instead of the git+ format.
<details>
<summary>🤖 Prompt for AI Agents</summary>
In @charts/pull-secret/README.md around lines 92 - 98, The README's Umbrella
Chart Integration uses a git+ repository URL in the dependencies block
(repository:
"git+https://github.com/openshift-hyperfleet/adapter-pull-secret@charts/pull-secret?ref=main")
which requires the helm-git plugin; update the Prerequisites section to document
that requirement by adding an entry for installing the helm-git plugin (include
the install command and recommended version), or instead change the repository
format to a standard Helm repo (OCI or HTTP chart repo) so the git+ URL is not
necessary; reference the dependencies block and the repository field when making
the change.
</details>
<!-- fingerprinting:phantom:triton:puma -->
<!-- This is an auto-generated comment by CodeRabbit -->
| securityContext: | ||
| {{- toYaml .Values.securityContext | nindent 10 }} | ||
| - name: pull-secret | ||
| image: {{ include "pull-secret.image" . }} |
There was a problem hiding this comment.
In other charts the image is defined in the deployment.yaml instead of doing it in a helper method
like:
image: "{{ .Values.image.registry }}/{{ .Values.image.repository }}:{{ .Values.image.tag | default .Chart.AppVersion }}"
I think it would be a good idea to keep all charts similar.
| volumes: | ||
| - name: tmp | ||
| emptyDir: {} |
There was a problem hiding this comment.
Is it required to declare a /tmp volume as emptyDir ?
Isn't there a /tmp folder by default for every container?
(Maybe is an OpenShift thing? I don't know)
charts/pull-secret/README.md
Outdated
| annotations: | ||
| iam.gke.io/gcp-service-account: "my-service-account@my-project.iam.gserviceaccount.com" |
There was a problem hiding this comment.
These should be not required now, as we are using principal:// for Workload Identity Federation instead of a concrete GCP service account
charts/pull-secret/README.md
Outdated
| # Create a pull-secret.json file with your credentials first | ||
| helm install pullsecret-job ./charts/pull-secret \ | ||
| --namespace hyperfleet-system \ | ||
| --create-namespace \ | ||
| --set env.gcpProjectId=my-project \ | ||
| --set env.clusterId=my-cluster-123 \ | ||
| --set env.pullSecretData='{"auths":{...}}' \ | ||
| --set gcp.projectId=my-project \ | ||
| --set cluster.id=my-cluster-123 \ | ||
| --set-file pullSecret.data=./pull-secret.json \ | ||
| --set image.tag=latest |
There was a problem hiding this comment.
I am confused here
This creates a job instead of a deployment, which would be the normal case for an adapter:
- an adapter runs always
- listens to events in a queue
- creates "adapter tasks" which are the jobs, that do the actual work of the adapter
- The cluster.id and gcp.projectID are not known until the adapter reads a pubsub message and extract the data from them
Is this intended for testing the adapter task job locally without spinning up an adapter process?
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@charts/pull-secret/configs/pull-secret-job-adapter-task.yaml`:
- Around line 14-16: activeDeadlineSeconds is hardcoded to 310 and can fall out
of sync with the configurable MAX_WAIT_TIME_SECONDS; change the manifest to use
a templated parameter (e.g., {{ .Values.pullSecret.maxWaitTimeSeconds }} or a
computed value) and set activeDeadlineSeconds to that parameter plus the 10s
buffer (activeDeadlineSeconds = maxWaitTimeSeconds + 10). Add the corresponding
helm/value param (pullSecret.maxWaitTimeSeconds or MAX_WAIT_TIME_SECONDS) to the
adapter config and update any code that computes the buffer so Job creation uses
the buffered value consistently.
♻️ Duplicate comments (2)
charts/pull-secret/templates/rbac.yaml (1)
56-59: Prevent ClusterRoleBinding from defaulting to the namespacedefaultServiceAccount.Line 58 relies on
pull-secret.serviceAccountName. Ifrbac.create=trueandserviceAccount.create=falsewithout an explicitserviceAccount.name, this can bind cluster-wide permissions to thedefaultSA. Please ensure the helper fails or requires an explicit name in that case.#!/bin/bash # Verify helper logic for pull-secret.serviceAccountName rg -n -C4 'define "pull-secret\.serviceAccountName"' charts/pull-secret/templates/_helpers.tplcharts/pull-secret/README.md (1)
117-123: Document the helm-git plugin requirement for umbrella chart integration.The
git+https://repository format requires the helm-git plugin to be installed. Users attempting to use this integration will encounter errors without proper guidance.Add to Prerequisites:
5. **helm-git plugin** (required for umbrella chart `git+` dependencies) ```bash helm plugin install https://github.com/aslafy-z/helm-gitAlternatively, consider publishing to a standard Helm repository or OCI registry to avoid this dependency. </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (6)</summary><blockquote> <details> <summary>charts/pull-secret/templates/NOTES.txt (1)</summary><blockquote> `44-49`: **Keep NOTES image output aligned with resolved image logic.** Line 45 prints `.Values.image.*` directly; if global overrides or helper-based image resolution are used elsewhere, NOTES may show a different image than what is actually deployed. Consider reusing the same helper as the deployment template here. </blockquote></details> <details> <summary>charts/pull-secret/configs/pull-secret-adapter.yaml (1)</summary><blockquote> `52-62`: **Avoid `:latest` defaults for adapter images.** Lines 55 and 61 default to `:latest`, which makes deployments non-reproducible and can introduce unexpected changes. Prefer pinned tags or digests (even if still overridable via env/values). </blockquote></details> <details> <summary>charts/pull-secret/templates/deployment.yaml (1)</summary><blockquote> `96-102`: **Make resource limits/requests configurable via values.** Resources are hardcoded, which limits flexibility for different environments. Users may need to tune resources based on message volume and cluster size. <details> <summary>♻️ Suggested changes</summary> In `deployment.yaml`: ```diff resources: - limits: - cpu: 500m - memory: 512Mi - requests: - cpu: 100m - memory: 128Mi + {{- toYaml .Values.resources | nindent 12 }}In
values.yaml, add under a new Resources section:# ============================================================================= # Resource Configuration # ============================================================================= resources: limits: cpu: 500m memory: 512Mi requests: cpu: 100m memory: 128Mi
charts/pull-secret/README.md (1)
7-24: Add language specifier to fenced code block.The architecture diagram code block should have a language specifier for accessibility and consistent rendering.
♻️ Suggested fix
-``` +```text ┌──────────────────────────────────────────────────────────────────┐
charts/pull-secret/values.yaml (2)
116-117: Use integer type formaxWaitTimeSeconds.The value is currently a string (
"300") but represents a numeric timeout. Using an integer would be more semantically correct and avoid potential type coercion issues.♻️ Suggested fix
# Maximum time to wait for job completion (seconds) - maxWaitTimeSeconds: "300" + maxWaitTimeSeconds: 300
71-89: Add validation for required broker configuration fields.The broker configuration values (
googlepubsub.projectId,googlepubsub.topic,googlepubsub.subscriptionfor Google Pub/Sub, andrabbitmq.urlfor RabbitMQ) are rendered directly into the ConfigMap and environment variables without validation. Currently, users can deploy with empty values that will fail at runtime.Add Helm's
requiredfunction inconfigmap-broker.yamlanddeployment.yamlwhere these values are rendered (e.g.,{{ required "projectId is required when broker.type is googlepubsub" .Values.broker.googlepubsub.projectId | quote }}), or create avalues.schema.jsonfor declarative validation.
| backoffLimit: 0 | ||
| # Maximum time to wait for k8s job completion (maxWaitTimeSeconds + 10 second buffer) | ||
| activeDeadlineSeconds: 310 |
There was a problem hiding this comment.
Align activeDeadlineSeconds with the configurable wait time.
Line 16 hardcodes 310s while MAX_WAIT_TIME_SECONDS is configurable; if users increase it, the Job may be killed early. Consider wiring activeDeadlineSeconds to a templated parameter (or compute a buffered value upstream) so both stay in sync.
🔧 Possible adjustment (also add a matching param in the adapter config)
- activeDeadlineSeconds: 310
+ # Keep in sync with MAX_WAIT_TIME_SECONDS (+ buffer if desired)
+ activeDeadlineSeconds: {{ .activeDeadlineSeconds }}🤖 Prompt for AI Agents
In `@charts/pull-secret/configs/pull-secret-job-adapter-task.yaml` around lines 14
- 16, activeDeadlineSeconds is hardcoded to 310 and can fall out of sync with
the configurable MAX_WAIT_TIME_SECONDS; change the manifest to use a templated
parameter (e.g., {{ .Values.pullSecret.maxWaitTimeSeconds }} or a computed
value) and set activeDeadlineSeconds to that parameter plus the 10s buffer
(activeDeadlineSeconds = maxWaitTimeSeconds + 10). Add the corresponding
helm/value param (pullSecret.maxWaitTimeSeconds or MAX_WAIT_TIME_SECONDS) to the
adapter config and update any code that computes the buffer so Job creation uses
the buffered value consistently.
2 participants
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.