-
Notifications
You must be signed in to change notification settings - Fork 4
HYPERFLEET-404 - test: add the cluster creation testcase #9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
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 |
WalkthroughAdds a new test case document at Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @testcases/cluster_lifecycle.md:
- Around line 155-250: The response example under the GET
/api/hyperfleet/v1/clusters "Response example" block is a single Cluster object
but must be a ClusterList; wrap the existing cluster JSON inside a ClusterList
with fields "kind": "ClusterList", "total": 1, "size": 1, "page": 1 and move the
cluster object into an "items" array; update the example JSON in the "Response
example" section so it reflects the ClusterList structure described in the
expectations.
🧹 Nitpick comments (3)
testcases/cluster_lifecycle.md (3)
155-155: Update Step 2 title to reflect the LIST operation.The current title "Verify Cluster API response" is ambiguous. This step tests the LIST endpoint, not a single cluster retrieval. Rename to something like "List all clusters" or "Verify cluster appears in list".
574-574: Hyphenate "Landing zone" for consistency.Use "Landing-zone" instead of "Landing zone" when used as a compound modifier before a noun.
582-582: Specify language for fenced code blocks.Lines 582 and 594 contain code blocks without a language identifier. Add
bashortextto the opening fence:🔧 Example fix for line 582
- ``` + ```bash NAME STATUS AGE 2nmg1slsk2t795jdu1mi1rk2iplgf4k3 Active 5mAlso applies to: 594-594
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
testcases/cluster_lifecycle.md
🧰 Additional context used
🪛 LanguageTool
testcases/cluster_lifecycle.md
[grammar] ~574-~574: Use a hyphen to join words.
Context: ...} ``` Expected Result: - Adapter created resources: - **Landing zone ...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
testcases/cluster_lifecycle.md
582-582: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
594-594: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (1)
testcases/cluster_lifecycle.md (1)
1-598: Overall structure and completeness—LGTM.The test case is well-organized with clear preconditions, sequential steps, and detailed validation criteria. The use of collapsible response examples aids readability. Once the Step 2 response inconsistency is resolved, this will be a solid foundation for automated or manual testing of cluster lifecycle operations.
09705a5 to
8e04582
Compare
There was a problem hiding this 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 @testcases/cluster_lifecycle.md:
- Line 590: Add language identifiers to the two fenced code blocks missing
specifiers (the blocks showing command output around the cluster/pods examples):
change the opening triple-backtick lines at the blocks referenced (near the
cluster NAME/STATUS table and the Pods verification example) to include a
language specifier such as "output" so they become ```output, leaving the
closing ``` unchanged; ensure both occurrences (the block before the cluster
table and the block before the pods table) are updated.
- Line 582: Change the heading text "Adapter created resources:" to use a
hyphenated compound adjective by replacing it with "Adapter-created resources:"
(or alternatively rephrase to "resources created by adapters") so the compound
adjective follows English hyphenation rules.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
testcases/cluster_lifecycle.md
🧰 Additional context used
🪛 LanguageTool
testcases/cluster_lifecycle.md
[grammar] ~582-~582: Use a hyphen to join words.
Context: ...} ``` Expected Result: - Adapter created resources: - **Landing zone ...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
testcases/cluster_lifecycle.md
590-590: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
602-602: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (1)
testcases/cluster_lifecycle.md (1)
1-606: Comprehensive and well-structured test case document.This test case provides excellent coverage of the cluster lifecycle workflow, including creation, listing, retrieval, adapter status monitoring, and resource verification via kubectl. The document is clear, thorough, and includes realistic examples with explicit validation checks for each step.
The progression from API operations (Steps 1-5) to infrastructure verification (Step 6) is logical and well-organized. Collapsible response examples keep the document readable while providing necessary detail. The validation checklist for each step makes it easy to implement automated or manual test scenarios.
testcases/cluster_lifecycle.md
Outdated
|
|
||
| ### Preconditions | ||
|
|
||
| 1. Hyperfleet API server is running and accessible at `http://localhost:8000` or other available url |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use a placeholder here for the API gateway URL instead of put localhost
testcases/cluster_lifecycle.md
Outdated
|
|
||
| # Feature: Cluster Lifecycle Management | ||
|
|
||
| ## Test Title: Cluster Lifecycle - Create and List Cluster |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use details and summary to folder cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The title is too lenient and makes it difficult to understand what we are verifying.
testcases/cluster_lifecycle.md
Outdated
|
|
||
| # Feature: Cluster Lifecycle Management | ||
|
|
||
| ## Test Title: Cluster Lifecycle - Create and List Cluster |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The case is not describing a scenario. Something like Create and list cluster will succeed should be better.
testcases/cluster_lifecycle.md
Outdated
|
|
||
| | **Field** | **Value** | | ||
| |-----------|-----------| | ||
| | **Title** | Cluster Lifecycle - Create and List Cluster | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad, I didn't realized there are Title and Feature in field
| **Action:** | ||
| Send POST request to create a new GCP cluster: | ||
|
|
||
| ```bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
details and summary folder the body will help.
testcases/cluster_lifecycle.md
Outdated
| ``` | ||
| </details> | ||
| - Verify response fields: | ||
| - `id` is generated and non-empty |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And in lower cases, and no duplication
testcases/cluster_lifecycle.md
Outdated
| - `kind` is "Cluster" | ||
| - `name` is "hp-gcp-cluster-1" | ||
| - `labels` contains "environment": "production" and "team": "platform" | ||
| - `created_by` and `updated_by` are "system" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is a valid checkpoint. You can clarify "system" is a placeholder now. I believe this feature will change with the auth introduced
testcases/cluster_lifecycle.md
Outdated
| - `name` is "hp-gcp-cluster-1" | ||
| - `labels` contains "environment": "production" and "team": "platform" | ||
| - `created_by` and `updated_by` are "system" | ||
| - `created_time` and `updated_time` are populated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Populated to what?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is hard to compare the time here. Just make sure the created_time/updated_time is not the default time.
testcases/cluster_lifecycle.md
Outdated
| </details> | ||
| - Response contains AdapterStatusList metadata: | ||
| - `kind` is "AdapterStatusList" | ||
| - `total` is 2 (two adapters) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be hardcoded here, we will deploy other adapters.
testcases/cluster_lifecycle.md
Outdated
| - `reason`: "Healthy" | ||
| - `message`: "All adapter operations completed successfully" | ||
| - `last_transition_time` is populated | ||
| - `data` contains namespace information: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There will be KSA being introduced later.
yasun1
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test case requires time waiting to get resources ready. It makes the case complex and big. I advice to separate them into pieces, etc. basic checking, adapters checking, k8s resources checking, and so on.
| | **Pos/Neg** | Positive | | ||
| | **Priority** | Critical | | ||
| | **Status** | Draft | | ||
| | **Automation** | Not Automated | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this? IMO, we could automate most of the cases, and if we draft the case, we will also input the running steps. If there arecannot-automated cases, it is better to separate them by folders.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need. The automation should happen after the case accepted, otherwise it will cause extra efforts.
testcases/cluster_lifecycle.md
Outdated
|
|
||
| # Feature: Cluster Lifecycle Management | ||
|
|
||
| ## Test Title: Cluster Lifecycle - Create and List Cluster |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The title is too lenient and makes it difficult to understand what we are verifying.
testcases/cluster_lifecycle.md
Outdated
| "baseDomain": "example.com" | ||
| } | ||
| } | ||
| }' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it better to put them into a test template folders to centralized manage the playloads?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good suggestion.
testcases/cluster_lifecycle.md
Outdated
| Send GET request to retrieve the cluster list: | ||
|
|
||
| ```bash | ||
| curl -X GET http://localhost:8000/api/hyperfleet/v1/clusters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you use LIST, you need to have a filter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
List and filter should be in another case. It's another scenario.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.Will use another case to cover list paramter function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to use LIST to get the just-created cluster, you need a filter to avoid there is no such cluster in the page/size=1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we might not need to do LIST here, but have the individual LIST to check whether there is duplicate ID in the whole DB.
testcases/cluster_lifecycle.md
Outdated
| - `updated_time` is populated | ||
| - `generation` is 1 | ||
| - `status.phase` is "NotReady" (initial state in MVP phase) | ||
| - `status.conditions` array exists with required fields |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what are required fields? The expected result should be clear.
| - Response contains all cluster metadata fields from Step 1 | ||
| - Cluster status contains adapter information: | ||
| - `status.conditions` array contains adapter status entries | ||
| - **LandingZoneAdapterSuccessful** condition exists:(example) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we need to wait for a while to wait this adapter status back, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.But according to the current result, it is very quick.I think we can add the expected time if it requires long time
testcases/cluster_lifecycle.md
Outdated
| - `name` is "hp-gcp-cluster-1" | ||
| - `labels` contains "environment": "production" and "team": "platform" | ||
| - `created_by` and `updated_by` are "system" | ||
| - `created_time` and `updated_time` are populated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is hard to compare the time here. Just make sure the created_time/updated_time is not the default time.
Yes.It is very long in markdown.We can separate it.But I am thinking, if we separate it, it will require to preapre cluster every time for every case. I prepare to put cluster API check and adapter status check via API in one case,k8s and other resource checking in a new case.WDYT? |
8e04582 to
49cd2ec
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In @testcases/cluster_lifecycle.md:
- Around line 136-155: The test currently asserts list pagination totals (e.g.,
total=1,size=1) against the /api/hyperfleet/v1/clusters response which will fail
in shared environments; replace those assertions with a presence check that
filters the returned .items by the cluster identifier or name (use CLUSTER_ID or
CLUSTER_NAME) and assert that a matching .id or .name exists in the items array
(e.g., via jq select(.id == $id) or equivalent), and remove or relax any exact
total/size checks; apply the same change to the other occurrence that checks
pagination (lines 234-255).
- Around line 27-43: The document omits how to obtain the {cluster_id} after
creating the cluster; update the "Create Cluster via API" step to show capturing
and exporting the returned cluster ID (e.g., run the curl request with -s and
parse the JSON response to extract the id) and provide a concrete example
command to export it as an environment variable (for example: response=$(curl -s
-X POST ... -d @testcases/templates/create_cluster_gcp.json); export
CLUSTER_ID=$(echo "$response" | jq -r '.id')). Ensure subsequent steps refer to
${CLUSTER_ID} (or clearly state replacing {cluster_id}) and add the same
guidance to the later occurrences referenced (lines around 256-264).
- Around line 545-618: Preconditions contains a broken fragment link to the
"Create cluster will success via API" header—fix the fragment in the
Preconditions item (the anchor after "see") to exactly match the target header
id/title; also add language qualifiers to the two fenced code blocks by changing
``` to ```text for both example outputs and hyphenate the phrase "Adapter
created resources" to "Adapter-created resources" in the Test Steps/Expected
Result section so lint MD051/MD040 and hyphenation issues are resolved.
In @testcases/templates/create_cluster_gcp.json:
- Around line 3-7: The template currently hard-codes name ("hp-gcp-cluster-1")
and labels.environment ("production"), which can cause collisions and incorrect
semantics; update the template fields (name and labels.environment) to be
clearly placeholder/parameterized values (e.g., a replaceable token or
environment variable reference) and add a brief comment indicating they must be
set per deployment; ensure the placeholder enforces uniqueness (e.g., include a
project/team prefix or timestamp token) and default to a non-production
environment value in documentation or templating logic.
- Around line 19-22: The spec.release.image currently points to the CVO
component image; update spec.release.image to the OpenShift release payload
image (one that contains release-manifests and image-references) instead of the
CVO component, e.g. use a payload tag like
quay.io/openshift-release-dev/ocp-release:4.14.z-x86_64; modify the JSON entry
under "release" (the "image" value) to the proper payload registry/tag so
runtime validation of spec.release.image succeeds.
🧹 Nitpick comments (1)
testcases/cluster_lifecycle.md (1)
360-484: Make adapter assertions tolerant to extra adapters / changing counts.
Hardcoding “at least 2” andtotal=2will get brittle as MVP evolves; better to assert required adapters exist, and ignore extras.Proposed wording/check
-- `total` matches the number of deployed adapters (at least 2 in MVP: landing-zone-adapter and dummy-validation-adapter) -- `size` matches `total` +- `items` contains entries for required adapters (at minimum): `landing-zone-adapter` and `dummy-validation-adapter` +- Additional adapters may appear and should not fail the test +Example check: +```bash +curl -sS "${API_URL}/api/hyperfleet/v1/clusters/${CLUSTER_ID}/statuses" | \ + jq -e ' + (.items | map(.adapter) | index("landing-zone-adapter")) != null and + (.items | map(.adapter) | index("dummy-validation-adapter")) != null + ' >/dev/null +```
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
testcases/cluster_lifecycle.mdtestcases/templates/create_cluster_gcp.json
🧰 Additional context used
🪛 LanguageTool
testcases/cluster_lifecycle.md
[grammar] ~594-~594: Use a hyphen to join words.
Context: ...} ``` Expected Result: - Adapter created resources: - **Landing zone ...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
testcases/cluster_lifecycle.md
570-570: Link fragments should be valid
(MD051, link-fragments)
602-602: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
614-614: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (1)
testcases/templates/create_cluster_gcp.json (1)
1-18: The template is correct as-is. The official API documentation intestcases/cluster_lifecycle.mdexplicitly shows the Cluster create payload using top-levelnameandlabelsfields, not ametadatawrapper. Both the request payload and response examples confirm this flat structure is the expected schema.Likely an incorrect or invalid review comment.
HYPERFLEET-404 :
Refine the previous e2e scenario and draft cluster creation via testcase format
Summary by CodeRabbit
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.