HYPERFLEET-606 - feat: Make adapter configuration mandatory #46
Conversation
- Remove default adapter values from pkg/config/adapter.go - NewAdapterRequirementsConfig() now returns error if env vars are missing - Update plugins to panic on startup if adapter config fails - Helm chart now requires adapters.cluster and adapters.nodepool values - Update documentation to reflect mandatory configuration - Update tests to use helper functions for adapter config
WalkthroughHelm templates now require and always render HYPERFLEET_CLUSTER_ADAPTERS and HYPERFLEET_NODEPOOL_ADAPTERS. Values/docs mark adapters as REQUIRED and include a new ServiceMonitor block. The Go config layer adds Adapters to ApplicationConfig and introduces NewAdapterRequirementsConfig() returning (config, error) with LoadFromEnv validating and parsing required env vars; initialization now fails on missing/invalid adapter env. Tests and integration harness set adapter env for runs; plugin constructors consume env.Config.Adapters instead of building their own adapter config. Sequence Diagram(s)sequenceDiagram
participant Helm as Helm (chart/templates)
participant K8s as Kubernetes Deployment
participant Pod as Container/App
participant Config as pkg/config (LoadAdapters)
participant Plugin as plugins (clusters/nodePools)
Helm->>K8s: Render Deployment with required HYPERFLEET_CLUSTER_ADAPTERS & HYPERFLEET_NODEPOOL_ADAPTERS
K8s->>Pod: Start Pod with env vars
Pod->>Config: Initialize adapters (NewAdapterRequirementsConfig / LoadFromEnv)
Config-->>Pod: return AdapterRequirementsConfig or error
alt adapters loaded
Pod->>Plugin: pass env.Config.Adapters to New*Service
Plugin-->>Pod: service initialized
else load failed
Pod->>Pod: log error and exit
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@charts/templates/deployment.yaml`:
- Around line 77-80: Replace the use of required on .Values.adapters.cluster and
.Values.adapters.nodepool (which treats [] as empty and fails) with a presence
check and safe rendering: check the adapters keys with hasKey (e.g., verify
.Values.adapters exists and hasKey .Values.adapters "cluster"/"nodepool" and
call fail with a clear message only if the key is missing), then render the env
values using default to allow an empty array (e.g., default []
.Values.adapters.cluster | toJson | quote). Update the
HYPERFLEET_CLUSTER_ADAPTERS and HYPERFLEET_NODEPOOL_ADAPTERS template lines to
use the presence check + default instead of required so [] is accepted at
runtime.
🧹 Nitpick comments (2)
pkg/config/adapter.go (1)
3-8: Avoid unstructuredlog.Printfin config loading.These logs bypass the context-aware structured logger (opid/accountID/tx_id). Consider dropping these success logs or wiring a contextual logger instead.
🔧 Minimal change (drop unstructured logs)
import ( "encoding/json" "fmt" - "log" "os" ) @@ - log.Printf("Loaded HYPERFLEET_CLUSTER_ADAPTERS from env: %v", clusterAdapters) @@ - log.Printf("Loaded HYPERFLEET_NODEPOOL_ADAPTERS from env: %v", nodepoolAdapters)As per coding guidelines: Use structured logging via logger.NewOCMLogger(ctx) with context information including [opid], [accountID], and [tx_id].
Also applies to: 39-58
pkg/services/node_pool_test.go (1)
21-27: Consider a shared test helper for adapter config.This helper duplicates the one in
pkg/services/cluster_test.go, which risks divergence if required adapters change. A small shared test util would keep defaults centralized.
Manual TestingValidated all critical scenarios:
|
|
/retest |
1 similar comment
|
/retest |
- Use hasKey + fail pattern instead of required function - Allows empty arrays [] when explicitly configured - Provides clear error messages when adapters not specified
fccf171 to
636905b
Compare
- Add Adapters field to ApplicationConfig - Add LoadAdapters() method called once during environment initialization - Update plugins to use env.Config.Adapters instead of creating their own - Remove duplicate NewAdapterRequirementsConfig() calls from plugins - Add required env vars to framework_test.go
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rh-amarin The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
c79db9d
into
openshift-hyperfleet:main
Summary
HYPERFLEET_CLUSTER_ADAPTERSandHYPERFLEET_NODEPOOL_ADAPTERSenvironment variables requiredChanges
pkg/config/adapter.go
NewAdapterRequirementsConfig()now returns(*AdapterRequirementsConfig, error)HYPERFLEET_CLUSTER_ADAPTERSorHYPERFLEET_NODEPOOL_ADAPTERSare not setplugins/clusters/plugin.go & plugins/nodePools/plugin.go
NewAdapterRequirementsConfig()charts/templates/deployment.yaml
requiredvalidation - Helm install fails if adapters are not providedcharts/values.yaml
[]tonulldocs/deployment.md
Test plan
make test- All 375 tests passmake lint- No lint issuesJira
https://issues.redhat.com/browse/HYPERFLEET-606
Summary by CodeRabbit
Breaking Changes
New Features
Documentation
Tests