Skip to content

Conversation

@yasun1
Copy link
Contributor

@yasun1 yasun1 commented Jan 26, 2026

Summary by CodeRabbit

  • New Features

    • Added a structured adapters configuration for specifying cluster and nodepool adapter requirements, replacing the previous inline environment-variable approach.
  • Documentation

    • Updated deployment docs with three configuration options: structured Helm values, Helm-based env var override, and direct CLI/env usage; includes defaults and examples for adapter configuration.

✏️ Tip: You can customize this high-level summary in your review settings.

@openshift-ci openshift-ci bot requested review from 86254860 and AlexVulaj January 26, 2026 04:04
@coderabbitai
Copy link

coderabbitai bot commented Jan 26, 2026

Walkthrough

This PR moves adapter configuration from directly setting environment variables to a structured Helm values block and updates the deployment template and docs accordingly. It adds an adapters section in values.yaml (with cluster and nodepool lists), conditionally emits HYPERFLEET_CLUSTER_ADAPTERS and HYPERFLEET_NODEPOOL_ADAPTERS (JSON-serialized and quoted) in the deployment template when those lists are present, and revises documentation to describe three configuration options (values YAML, Helm env overrides, or direct env vars).

Sequence Diagram(s)

sequenceDiagram
    participant Dev as Developer (values.yaml / Helm CLI)
    participant Helm as Helm Template Engine
    participant K8s as Kubernetes API/Deployment
    participant Pod as Container Runtime (app)

    Dev->>Helm: Provide values.yaml (adapters.cluster/adapters.nodepool) or env overrides
    Helm->>K8s: Render deployment.yaml (conditionally include HYPERFLEET_* env vars) and apply
    K8s->>Pod: Schedule Pod with rendered env vars
    Pod->>Pod: Application reads HYPERFLEET_CLUSTER_ADAPTERS / HYPERFLEET_NODEPOOL_ADAPTERS env
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

Suggested labels

lgtm, approved

Suggested reviewers

  • aredenba-rh
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: refactoring adapter requirements from inline environment variables to a YAML table format. It's specific, concise, and clearly summarizes the primary objective of the changeset.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In `@charts/templates/deployment.yaml`:
- Around line 77-85: The template currently skips adapter env vars when users
set empty lists because Helm treats empty slices as false; update the
conditional checks in charts/templates/deployment.yaml to test for key existence
and nil explicitly (e.g., use hasKey .Values.adapterRequirements "cluster" and
.Values.adapterRequirements.cluster != nil, and similarly for "nodepool") so an
explicit [] value is rendered as HYPERFLEET_*_ADAPTERS: "[]"; also ensure
values.yaml can default these keys to null (not undefined) so users can set []
to clear defaults and the template will emit the empty list JSON via the
existing toJson | quote pipeline.

In `@docs/deployment.md`:
- Around line 96-120: The markdown uses bold lines as section titles for the
three options, triggering MD036; replace those bolded option lines with proper
Markdown headings (e.g., "### Option 1: Using structured values (Helm only,
recommended)", "### Option 2: Using environment variables in Helm", "### Option
3: Direct environment variable (non-Helm)") so the document structure is
semantic and lint-friendly; update the surrounding blocks (code fences and
descriptions) to remain under each new heading and ensure heading level matches
the surrounding document hierarchy.

Comment on lines 77 to 85
{{- if .Values.adapterRequirements }}
{{- if .Values.adapterRequirements.cluster }}
- name: HYPERFLEET_CLUSTER_ADAPTERS
value: {{ .Values.adapterRequirements.cluster | toJson | quote }}
{{- end }}
{{- if .Values.adapterRequirements.nodepool }}
- name: HYPERFLEET_NODEPOOL_ADAPTERS
value: {{ .Values.adapterRequirements.nodepool | toJson | quote }}
{{- end }}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Allow explicit empty adapter lists (currently impossible).
With if .Values.adapterRequirements.cluster, an empty list evaluates false, so users cannot explicitly set [] to clear defaults. Consider checking for nil and defaulting to null in values.yaml so empty lists can be emitted as [].

🛠️ Suggested change (template)
-        {{- if .Values.adapterRequirements.cluster }}
+        {{- if ne .Values.adapterRequirements.cluster nil }}
         - name: HYPERFLEET_CLUSTER_ADAPTERS
           value: {{ .Values.adapterRequirements.cluster | toJson | quote }}
         {{- end }}
-        {{- if .Values.adapterRequirements.nodepool }}
+        {{- if ne .Values.adapterRequirements.nodepool nil }}
         - name: HYPERFLEET_NODEPOOL_ADAPTERS
           value: {{ .Values.adapterRequirements.nodepool | toJson | quote }}
         {{- end }}
🛠️ Suggested change (values.yaml defaults)
 adapterRequirements:
   # Required adapters for cluster "Ready" state
   # Default: ["validation", "dns", "pullsecret", "hypershift"]
-  cluster: []
+  cluster: null
   # Required adapters for nodepool "Ready" state
   # Default: ["validation", "hypershift"]
-  nodepool: []
+  nodepool: null
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
{{- if .Values.adapterRequirements }}
{{- if .Values.adapterRequirements.cluster }}
- name: HYPERFLEET_CLUSTER_ADAPTERS
value: {{ .Values.adapterRequirements.cluster | toJson | quote }}
{{- end }}
{{- if .Values.adapterRequirements.nodepool }}
- name: HYPERFLEET_NODEPOOL_ADAPTERS
value: {{ .Values.adapterRequirements.nodepool | toJson | quote }}
{{- end }}
{{- if .Values.adapterRequirements }}
{{- if ne .Values.adapterRequirements.cluster nil }}
- name: HYPERFLEET_CLUSTER_ADAPTERS
value: {{ .Values.adapterRequirements.cluster | toJson | quote }}
{{- end }}
{{- if ne .Values.adapterRequirements.nodepool nil }}
- name: HYPERFLEET_NODEPOOL_ADAPTERS
value: {{ .Values.adapterRequirements.nodepool | toJson | quote }}
{{- end }}
🤖 Prompt for AI Agents
In `@charts/templates/deployment.yaml` around lines 77 - 85, The template
currently skips adapter env vars when users set empty lists because Helm treats
empty slices as false; update the conditional checks in
charts/templates/deployment.yaml to test for key existence and nil explicitly
(e.g., use hasKey .Values.adapterRequirements "cluster" and
.Values.adapterRequirements.cluster != nil, and similarly for "nodepool") so an
explicit [] value is rendered as HYPERFLEET_*_ADAPTERS: "[]"; also ensure
values.yaml can default these keys to null (not undefined) so users can set []
to clear defaults and the template will emit the empty list JSON via the
existing toJson | quote pipeline.

Comment on lines 96 to 120
**Option 1: Using structured values (Helm only, recommended)**
```yaml
# values.yaml
adapterRequirements:
cluster:
- validation
- dns
- pullsecret
- hypershift
nodepool:
- validation
- hypershift
```

**Option 2: Using environment variables in Helm**
```yaml
# values.yaml
env:
- name: HYPERFLEET_CLUSTER_ADAPTERS
value: '["validation","dns","pullsecret","hypershift"]'
- name: HYPERFLEET_NODEPOOL_ADAPTERS
value: '["validation","hypershift"]'
```

**Option 3: Direct environment variable (non-Helm)**
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Use headings instead of bold lines for options (MD036).
markdownlint flags emphasis-as-heading here; headings improve structure and fix lint.

✏️ Proposed fix
-**Option 1: Using structured values (Helm only, recommended)**
+#### Option 1: Using structured values (Helm only, recommended)

-**Option 2: Using environment variables in Helm**
+#### Option 2: Using environment variables in Helm

-**Option 3: Direct environment variable (non-Helm)**
+#### Option 3: Direct environment variable (non-Helm)
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)

96-96: Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)


110-110: Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)


120-120: Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)

🤖 Prompt for AI Agents
In `@docs/deployment.md` around lines 96 - 120, The markdown uses bold lines as
section titles for the three options, triggering MD036; replace those bolded
option lines with proper Markdown headings (e.g., "### Option 1: Using
structured values (Helm only, recommended)", "### Option 2: Using environment
variables in Helm", "### Option 3: Direct environment variable (non-Helm)") so
the document structure is semantic and lint-friendly; update the surrounding
blocks (code fences and descriptions) to remain under each new heading and
ensure heading level matches the surrounding document hierarchy.

- hypershift
```

**Option 2: Using environment variables in Helm**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean it also support Option 2 and 3?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, also support Option 2 and 3

@86254860
Copy link
Contributor

/lgtm

@openshift-ci
Copy link

openshift-ci bot commented Jan 27, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 86254860

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot bot merged commit a529611 into openshift-hyperfleet:main Jan 27, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants