docs(RHIDP-15201): add openspec artifacts for K8s config migration - #4176
docs(RHIDP-15201): add openspec artifacts for K8s config migration#4176gabemontero wants to merge 6 commits into
Conversation
Add proposal, design, tasks, and behavioral spec for migrating K8s
credentials and plugin config to app-config.yaml in the
kserve-kubeflow-connector-backend plugin.
a) Original artifacts:
- proposal.md: Why, starting point, what changes, capabilities,
non-goals, impact (~+180/-90 lines across 9 files)
- design.md: 8 decisions (D1-D8) — two config approaches following
OCM pattern, remove baseUrl, merge ConnectorConfig into
ReconcilerConfig, loadFromOptions, serviceAccount-only auth,
config.d.ts schema, precedence chain, K8s RBAC example
- tasks.md: 10 task groups covering removal, merge, K8s fields,
KubeConfig implementation, kubernetesPluginRef lookup, schema,
app-config example, RBAC YAML, logger, verification
- spec.md: behavioral specs for direct config, kubernetesPluginRef,
config precedence, baseUrl removal, ConnectorConfig merge,
config schema, K8s RBAC, build quality
b) Fixes from openspec-audit-change adversarial audit (15 findings):
- Fixed kubernetesPluginRef fallback contradiction (CRITICAL):
design D7, spec scenarios, and tasks 5.2-5.3 now show
fall-through to direct config when ref lookup fails
- Fixed safeGetOptionalString usage (CRITICAL): tasks 5.2/5.3 now
use safeGetOptionalString for all K8s config reads per AGENTS.md
ConfigReader edge case
- Fixed optionality syntax (CRITICAL): normalized to '?: string'
consistently between design.md and tasks.md
- Added cross-change reconciliation section to design.md for
remove-kfmr-client-code and techdocs-modelcard-finalization
- Clarified task 2.3 logger source (from init deps)
- Softened D1 'ignored' wording to 'takes precedence' with D7
cross-reference for kubernetesPluginRef fallback behavior
- Removed redundant '?? undefined' on getOptionalBoolean in task 5.3
c) Fixes from author review:
- Added KUBECONFIG env var and ~/.kube/config (from oc login) as
peer local dev options alongside K8S_TOKEN across all four files
- Expanded D7 precedence to merge env var steps into single
'local dev — loadFromDefault()' step with three sub-bullets
- Added spec scenarios for KUBECONFIG env var, ~/.kube/config from
oc login, config overriding kubeconfig, and no-config failure
- Added verification task 10.9 for KUBECONFIG env var
Assisted-by: Claude Opus 4.6
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: gabemontero <gmontero@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 3:57 PM UTC · Completed 4:15 PM UTC |
PR Summary by QodoAdd openspec docs for K8s config migration to app-config.yaml
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4176 +/- ##
=======================================
Coverage 58.12% 58.12%
=======================================
Files 2422 2422
Lines 96490 96490
Branches 26870 26870
=======================================
Hits 56081 56081
Misses 38956 38956
Partials 1453 1453
*This pull request uses carry forward flags. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
Code Review by Qodo
1.
|
rhdh-qodo-merge
Bot
added
the
documentation
ReviewFindingsMedium
Low
Previous runReviewFindingsMedium
Low
Previous run (2)ReviewFindingsMedium
Low
Labels: PR adds only documentation/planning artifacts (design, proposal, spec, tasks) for K8s config migration |
fullsend-ai-review
Bot
added
requires-manual-review
Review responseThanks for the review. Addressing 7 of the 10 findings: Fixing (2 Medium + 5 Low)
Skipping (3 Low)
|
…ation openspecs Fix 2 medium and 5 low findings from fullsend review agent (PR redhat-developer#4176). Medium fixes: - Task 5.1: changed getOptionalString to safeGetOptionalString for kubernetesPluginRef read, consistent with Task 5 note and 5.2/5.3 - Task 8.1 + design D8: removed watch verb from routes RBAC rule — setupCatalogRoute only does list, no informer/watch on routes Low fixes: - Task 5 note: clarified safeGetOptionalString must return undefined (not empty string) on error — the url-reader variant returns '' which would pass truthiness checks and send empty URL to loadFromOptions - Design D7 title: updated from 4-level to 3-level to match body (env vars merged into loadFromDefault step during author review) - Task 5.3 + spec.md: added authProvider validation per D5 — log warning when kubernetesPluginRef cluster uses non-serviceAccount authProvider (renumbered 5.3-5.5) - Added .openspec.yaml metadata file to match other change directories - Normalized Non-goals to Non-Goals casing in proposal.md Skipped (3 low): - Empty string truthiness: covered by safeGetOptionalString return fix - Task 9 scope: justified follow-on from setupInformer signature change - Absolute paths: intentional for team cross-reference workflow Assisted-by: Claude Opus 4.6 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: gabemontero <gmontero@redhat.com>
|
🤖 Review · ❌ Terminated · Started 5:07 PM UTC · Ended 5:25 PM UTC |
Qodo review responseAddressing 2 of the 4 findings (the other 2 were already fixed in the fullsend review commit): Fixed
Already resolved
|
…n openspecs Fix 2 findings from qodo review (PR redhat-developer#4176). 1. Task file paths: added plugins/ prefix to all plugin references across tasks.md, design.md, and proposal.md to match workspace- relative layout (e.g., plugins/kserve-kubeflow-connector-backend/ instead of bare kserve-kubeflow-connector-backend/). Matches convention in remove-kfmr-client-code/tasks.md. 2. Author-specific absolute paths: replaced /home/gmontero/... local filesystem paths with GitHub URLs to upstream files in design.md (OCM config.ts, kubernetes.ts, K8s plugin config.d.ts) and proposal.md (consolidated into Reference links, removed Local refs line). Assisted-by: Claude Opus 4.6 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: gabemontero <gmontero@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 5:07 PM UTC · Completed 5:25 PM UTC |
Second fullsend review responseAddressing 4 of the 7 findings from the second review run (3 were already fixed in prior commits): Fixed
Already fixed in prior commits
Skipped
|
Fix 1 medium and 3 low findings from the second fullsend review run. Medium: - Remove k8sToken from ReconcilerConfig, replaced by serviceAccountToken as the single token field. Updated D3 to explicitly document the removal. Updated Task 4.3 to store resolved token on config.serviceAccountToken and update all k8sToken readers. Low fixes: - Partial config warning: added logger.warn when only one of url or serviceAccountToken is present (D4 code sketch, Task 4.1 code, and two new spec scenarios for missing-token and missing-url cases) - D2 rationale: removed inaccurate claim about DiscoveryService and AuthService sharing a dependency; simplified to 'hardcoded baseUrl cannot help when the service is unreachable' - Non-goals casing: reverted to 'Non-goals' (lowercase g) in proposal.md to match convention in all 4 other proposals Skipped: - Local filesystem paths: already fixed in prior commit - Novel Canonical Touchpoints field names: Reference field is useful for upstream repo links not present in other proposals Assisted-by: Claude Opus 4.6 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: gabemontero <gmontero@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 5:39 PM UTC · Completed 5:55 PM UTC |
johnmcollier
left a comment
johnmcollier
left a comment
There was a problem hiding this comment.
Looks good, no concerns outside of the qodo/fullsend reviews, just a couple very minor nits/questions.
Fix 3 inline comments from PR redhat-developer#4176 review by @johnmcollier: - Changed skipTLSVerify from true to false in the 'all fields' scenario so caData is meaningful (caData is redundant when TLS verification is skipped) - Replaced hardcoded example values (URL, token, caData strings) in THEN assertions with references to 'the configured value' — spec should describe behavior, not test data - Changed GIVEN values from example strings to descriptive labels (e.g., 'set to a cluster API endpoint' instead of 'https://api.my-cluster.example.com:6443') Assisted-by: Claude Opus 4.6 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: gabemontero <gmontero@redhat.com>
Fix 1 medium and 2 low findings from the third fullsend review run.
Medium:
- kubernetesPluginRef lookup break condition: replaced
reconcilerConfig.url truthiness check with a 'matched' flag to
prevent the outer locator loop from continuing past a name-matched
cluster that has no url. Without this, a cluster with the same name
in a different locator could be incorrectly matched. Updated both
tasks.md Task 5.2 code and design.md D7 pseudocode. Also added a
distinct warning when the ref matches but the cluster has no url.
Low fixes:
- safeGetOptionalString for name comparison: changed
cluster.getOptionalString('name') to
safeGetOptionalString(cluster, 'name') in Task 5.2 and D7
pseudocode for consistency with the Task 5 convention note
- spec coverage gap: added scenario for kubernetesPluginRef matching
a cluster with incomplete K8s fields (url but no token), documenting
the D4+D7 composition (partial-config warning then loadFromDefault)
Skipped:
- RBAC get verb: standard K8s practice to grant get+list together;
read-only verb with no security impact
- Task 9 scope creep: justified follow-on, flagged for third time
Assisted-by: Claude Opus 4.6
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: gabemontero <gmontero@redhat.com>
|
Review responses — John Collier + third fullsend runJohn Collier's review (spec.md)Fixed all 3 inline comments on the "Direct config with all fields" scenario:
Third fullsend reviewFixing 3 of 5 findings:
Skipped (same as prior rounds):
|
|
🤖 Review · Started 6:06 PM UTC |
2 participants
Add proposal, design, tasks, and behavioral spec for migrating K8s credentials and plugin config to app-config.yaml in the kserve-kubeflow-connector-backend plugin.
a) Original artifacts:
b) Fixes from openspec-audit-change adversarial audit (15 findings):
c) Fixes from author review:
Assisted-by: Claude Opus 4.6
Hey, I just made a Pull Request!
✔️ Checklist