update the adapter-config-template according to latest implementation #45
Conversation
WalkthroughThe PR converts the adapter config to an MVP schema: adds an optional Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
xueli181114
force-pushed
the
adapter-design
branch
from
438a2e8 to
8d1549b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
hyperfleet/components/adapter/framework/adapter-config-template-MVP.yaml (1)
75-97: Template appears to be missingimageTagparam referenced by the PR summary
Underspec.params(Lines 75-97), I don’t see the optional env-sourcedimageTag. If the implementation expects it (or other docs reference it), please add it here to keep the template “latest”.hyperfleet/components/adapter/framework/adapter-frame-design.md (1)
381-385: Auth/token guidance conflicts with MVP status reporting (no Authorization header / token removed)
This section still states “Token fromHYPERFLEET_API_TOKEN… (for Authorization header)” (Lines 384-385). If auth is explicitly post-MVP/future, label it as such; otherwise update/remove to match the MVP contract/template.
🧹 Nitpick comments (2)
hyperfleet/components/adapter/framework/adapter-status-contract.md (1)
9-10: Link target is a YAML, not a doc—consider renaming link text for clarity
“Adapter Config Template MVP” points to a YAML file; consider “Adapter Config Template (MVP YAML)” to set expectations.hyperfleet/components/adapter/framework/adapter-config-template-MVP.yaml (1)
164-232:observed_timeformat may diverge from examples (UTC “Z” vs numeric offset)
{{ now | date "2006-01-02T15:04:05Z07:00" }}yields RFC3339 (good), but not necessarily a...Zsuffix like other examples. If you want normalized UTC to match docs/contracts, consider formatting in UTC (or clearly state offsets are expected).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
hyperfleet/components/adapter/framework/adapter-config-template-MVP.yaml(6 hunks)hyperfleet/components/adapter/framework/adapter-deployment.md(2 hunks)hyperfleet/components/adapter/framework/adapter-design-decisions.md(3 hunks)hyperfleet/components/adapter/framework/adapter-frame-design.md(9 hunks)hyperfleet/components/adapter/framework/adapter-integration-tests.md(3 hunks)hyperfleet/components/adapter/framework/adapter-status-contract.md(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- hyperfleet/components/adapter/framework/adapter-deployment.md
- hyperfleet/components/adapter/framework/adapter-design-decisions.md
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
hyperfleet/components/adapter/framework/adapter-frame-design.md
111-111: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
🔇 Additional comments (3)
hyperfleet/components/adapter/framework/adapter-config-template-MVP.yaml (2)
26-33: Nice addition: explicit CEL optional chaining guidance
Examples using?.and.orValue()will prevent lots of “missing field” failures in templates.
106-126: Precondition gating semantics: double-checkclusterPhase == NotReadyis intended
Template now blocks unlessclusterPhaseequalsNotReady(Lines 123-126). If adapters are supposed to act duringProvisioning/Installing(common in the other docs), this example will be misleading.hyperfleet/components/adapter/framework/adapter-frame-design.md (1)
109-115: Config model update summary reads clean and matches MVP direction
TheConfig/ParamConfig/PreconditionConfig/ResourceConfig/PostConfigbullets are a helpful quick map.
| - `PreconditionConfig`: Defines API calls and conditions for resource creation | ||
| - `PostConfig`: Defines status evaluation and reporting logic | ||
| - `Config`: Root configuration structure matching adapter-config-template-MVP.yaml | ||
| - `ParamConfig`: Defines parameter extraction rules (env.* or event.* sources) |
There was a problem hiding this comment.
Fix markdownlint MD037 (spaces inside emphasis)
Static analysis flags spacing inside emphasis markers at Line 111; please adjust formatting to satisfy markdownlint.
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
111-111: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
🤖 Prompt for AI Agents
In hyperfleet/components/adapter/framework/adapter-frame-design.md around line
111, markdownlint MD037 reports spaces inside emphasis markers; edit line 111 to
remove any spaces immediately inside the emphasis markers (e.g., change "*
ParamConfig *" to "*ParamConfig*" or replace with inline code backticks without
extra spaces like `ParamConfig`) so there are no spaces between the emphasis
delimiters and the text.
| **API Endpoints to Mock**: | ||
| - `GET /api/v1/clusters/{clusterId}` - Return cluster object | ||
| - `GET /api/v1/clusters/{clusterId}/statuses` - Return existing status or 404 | ||
| - `POST /api/v1/clusters/{clusterId}/statuses` - Create status | ||
| - `PATCH /api/v1/clusters/{clusterId}/statuses/{statusId}` - Update status | ||
| - `GET /api/hyperfleet/v1/clusters/{clusterId}` - Return cluster object | ||
| - `GET /api/hyperfleet/v1/clusters/{clusterId}/statuses` - Return existing status or 404 | ||
| - `POST /api/hyperfleet/v1/clusters/{clusterId}/statuses` - Create/upsert status | ||
| - `PATCH /api/hyperfleet/v1/clusters/{clusterId}/statuses/{statusId}` - Update status | ||
|
|
There was a problem hiding this comment.
Doc still contains stale /api/v1/... assertion paths—should be /api/hyperfleet/v1/...
You updated the mocked endpoints list to /api/hyperfleet/v1/... (Lines 100-103), but several assertion snippets still use /api/v1/clusters/... (e.g., Line 524, 559, 709). These should be aligned to avoid copy/paste test bugs.
Also applies to: 521-528, 556-563, 706-713
🤖 Prompt for AI Agents
In hyperfleet/components/adapter/framework/adapter-integration-tests.md around
lines 99-104 and in the assertion snippets at 521-528, 556-563, 706-713 (also
specific occurrences at ~524, ~559, ~709), there are stale paths using
/api/v1/clusters/... while the mocked endpoints use /api/hyperfleet/v1/...;
update all assertion URLs to use /api/hyperfleet/v1/... (search for any
remaining /api/v1/clusters and replace with /api/hyperfleet/v1/clusters), run a
quick grep to ensure no other occurrences remain, and keep the path segments and
parameter placeholders identical to existing mock definitions.
| **Base URL**: `{hyperfleetApiBaseUrl}/api/hyperfleet/{hyperfleetApiVersion}/clusters/{clusterId}/statuses` | ||
|
|
||
| **Method**: |
There was a problem hiding this comment.
Doc is inconsistent with MVP: post.parameters + Authorization header + old endpoint examples remain
You updated the base URL (Line 18), but later sections still reference post.parameters.build (Line 529), include an Authorization: Bearer {hyperfleetApiToken} header (Lines 612-615), and show an example request path POST /api/v1/... (Line 625) instead of /api/hyperfleet/{version}/.... This will mislead adapter authors.
Also applies to: 525-605, 608-665
|
|
||
| ## Post-MVP Enhancements | ||
|
|
||
| ### KEDA Autoscaling |
There was a problem hiding this comment.
Do we need to sketch future ideas here?
There are other ways like using prometheus for getting the pending messages.
There was a problem hiding this comment.
That comes to my mind when I working on the helm charts deployment. How about keeping it for further reference? I won't say it's a decision, just some idea we can have in the further enhancement.
|
/lgtm |
2 participants
Summary by CodeRabbit
New Features
Refactor
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.