CNTRLPLANE-3897: Add product-cli unit tests for create nodepool - #9121
CNTRLPLANE-3897: Add product-cli unit tests for create nodepool#9121mehabhalodiya wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@mehabhalodiya: This pull request references CNTRLPLANE-3897 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds unit tests for agent, AWS, Azure, KubeVirt, and OpenStack nodepool create commands. The tests verify command names, registered flags, provider-specific defaults, flag parsing, option validation and completion, nodepool platform specification updates, fixture comparisons, agent label selectors, and KubeVirt’s excluded Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
area/platform/aws
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9121 +/- ##
==========================================
+ Coverage 44.51% 44.65% +0.13%
==========================================
Files 774 774
Lines 97003 97059 +56
==========================================
+ Hits 43185 43340 +155
+ Misses 50830 50720 -110
- Partials 2988 2999 +11 see 12 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
SummaryThis PR adds unit tests for the
FeedbackThe current approach validates flag registration and defaults, but it doesn't test the actual behavior of the create commands — i.e. what the commands produce when invoked. The existing cluster create commands in
This fixture-based approach has key advantages:
Suggestion: Consider aligning these nodepool create tests with the existing cluster create test pattern — using fixture YAML files to validate the rendered output of each platform's create command, similar to what's done in — Summary generated from Slack discussion by Chai Bot |
f4d2fe2 to
260d2f2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
product-cli/cmd/nodepool/agent/create_test.go (2)
57-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThese cases exercise the upstream options struct, not the product-cli command wiring.
NewAgentPlatformCreateOptions(&cobra.Command{})plus a direct field assignment skipsNewCreateCommand's flag registration, so a product-cli wiring regression (e.g. flag renamed or unbound) wouldn't fail here. Consider parsing throughcmd.Flags()instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@product-cli/cmd/nodepool/agent/create_test.go` around lines 57 - 58, Update the test setup around NewAgentPlatformCreateOptions to exercise the product-cli command wiring by creating the command through NewCreateCommand and parsing the relevant value from cmd.Flags(), rather than assigning AgentLabelSelector directly. Preserve the existing test cases while ensuring flag registration, naming, and binding regressions are covered.
53-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCollapse the three selector cases into a data-driven table.
These three entries differ only by the selector string; the
verify func(t *testing.T)shape forces the whole body to be duplicated. A small struct field (selector string) with a shared body would keep them parameterized.As per coding guidelines, "Use table-driven tests where possible".
♻️ Sketch
- "When flags are parsed with empty label selector, it should generate correct nodepool": { - verify: func(t *testing.T) { - ctx := t.Context() - platformOpts := hypershiftagent.NewAgentPlatformCreateOptions(&cobra.Command{}) - platformOpts.AgentLabelSelector = "" - ... + // tests map[string]struct{ selector string; verify func(t *testing.T, selector string) } + "When flags are parsed with empty label selector, it should generate correct nodepool": { + selector: "", + }, + "When flags are parsed with single label selector, it should generate correct nodepool": { + selector: "size=large", + }, + "When flags are parsed with multi label selector, it should generate correct nodepool": { + selector: "size=large,zone notin (az1,az2)", + },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@product-cli/cmd/nodepool/agent/create_test.go` around lines 53 - 121, Refactor the three selector-specific cases in the test table into a data-driven form using a selector field, with one shared verification function that applies the field to AgentLabelSelector and performs the existing node pool update and fixture comparison. Preserve the empty, single, and multi-selector values as separate table entries.Source: Coding guidelines
product-cli/cmd/nodepool/aws/create_test.go (2)
212-214: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
ptr.Toover a localstrPtrhelper.
k8s.io/utils/ptris already used elsewhere in the repo (e.g.cmd/nodepool/kubevirt/create.go), and a package-levelstrPtrrisks colliding with other helpers in this package.♻️ Proposed change
-func strPtr(s string) *string { - return &s -}Then replace call sites:
ID: ptr.To("subnet-default"),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@product-cli/cmd/nodepool/aws/create_test.go` around lines 212 - 214, Remove the package-level strPtr helper and use k8s.io/utils/ptr.To at every affected call site, including the ID assignment in the node pool test. Add or reuse the existing ptr import without introducing another local pointer helper.
66-201: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMinimal and full cases are ~90% identical.
Everything except
argsis duplicated between the two blocks. Extracting a helper that takesargsand a fixture-relevant name would cut this in half and keep future flag additions in one place.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@product-cli/cmd/nodepool/aws/create_test.go` around lines 66 - 201, Refactor the duplicated test setup in the minimal and full configuration cases into a shared helper that accepts the argument list and fixture-relevant test name. Move the common flag parsing, option validation/completion, nodepool and hosted-cluster construction, update via UpdateNodePool, and fixture comparison into that helper; keep each test case focused on supplying its distinct args and name.product-cli/cmd/nodepool/azure/create_test.go (1)
141-157: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win"Full configuration" cases parse only a subset of the flags each command registers. Each provider asserts an exact registered-flag set, then exercises a "full" case that skips several of those flags, so conditional branches in the spec builders never appear in any fixture. Keeping the full case in sync with the asserted flag list closes the gap.
product-cli/cmd/nodepool/azure/create_test.go#L141-L157: add--disk-encryption-set-id,--diagnostics-storage-account-type, and--diagnostics-storage-account-urito cover theDiagnostics/UserManagedbranch.product-cli/cmd/nodepool/kubevirt/create_test.go#L156-L170: add--root-volume-cache-strategy,--vm-node-selector, and--host-device-nameto cover the correspondingNodePoolPlatformbranches.product-cli/cmd/nodepool/openstack/create_test.go#L106-L113: add--openstack-node-additional-portsoAdditionalPortsis non-empty in at least one fixture.As per coding guidelines, "Unit test any code changes and additions."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@product-cli/cmd/nodepool/azure/create_test.go` around lines 141 - 157, Update the full-configuration fixtures so they exercise every relevant registered flag: in product-cli/cmd/nodepool/azure/create_test.go:141-157 add the disk encryption set ID and both diagnostics storage account flags; in product-cli/cmd/nodepool/kubevirt/create_test.go:156-170 add the root volume cache strategy, VM node selector, and host device name flags; and in product-cli/cmd/nodepool/openstack/create_test.go:106-113 add the openstack additional port flag. Use the existing test symbols and valid values so the Azure Diagnostics/UserManaged, KubeVirt NodePoolPlatform, and OpenStack AdditionalPorts branches are covered, then update or add unit assertions as needed.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@product-cli/cmd/nodepool/agent/create_test.go`:
- Around line 57-58: Update the test setup around NewAgentPlatformCreateOptions
to exercise the product-cli command wiring by creating the command through
NewCreateCommand and parsing the relevant value from cmd.Flags(), rather than
assigning AgentLabelSelector directly. Preserve the existing test cases while
ensuring flag registration, naming, and binding regressions are covered.
- Around line 53-121: Refactor the three selector-specific cases in the test
table into a data-driven form using a selector field, with one shared
verification function that applies the field to AgentLabelSelector and performs
the existing node pool update and fixture comparison. Preserve the empty,
single, and multi-selector values as separate table entries.
In `@product-cli/cmd/nodepool/aws/create_test.go`:
- Around line 212-214: Remove the package-level strPtr helper and use
k8s.io/utils/ptr.To at every affected call site, including the ID assignment in
the node pool test. Add or reuse the existing ptr import without introducing
another local pointer helper.
- Around line 66-201: Refactor the duplicated test setup in the minimal and full
configuration cases into a shared helper that accepts the argument list and
fixture-relevant test name. Move the common flag parsing, option
validation/completion, nodepool and hosted-cluster construction, update via
UpdateNodePool, and fixture comparison into that helper; keep each test case
focused on supplying its distinct args and name.
In `@product-cli/cmd/nodepool/azure/create_test.go`:
- Around line 141-157: Update the full-configuration fixtures so they exercise
every relevant registered flag: in
product-cli/cmd/nodepool/azure/create_test.go:141-157 add the disk encryption
set ID and both diagnostics storage account flags; in
product-cli/cmd/nodepool/kubevirt/create_test.go:156-170 add the root volume
cache strategy, VM node selector, and host device name flags; and in
product-cli/cmd/nodepool/openstack/create_test.go:106-113 add the openstack
additional port flag. Use the existing test symbols and valid values so the
Azure Diagnostics/UserManaged, KubeVirt NodePoolPlatform, and OpenStack
AdditionalPorts branches are covered, then update or add unit assertions as
needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 66434799-aae6-4614-afbd-dc5fec827f2f
⛔ Files ignored due to path filters (11)
product-cli/cmd/nodepool/agent/testdata/zz_fixture_TestNewCreateCommand_When_flags_are_parsed_with_empty_label_selector__it_should_generate_correct_nodepool.yamlis excluded by!**/testdata/**product-cli/cmd/nodepool/agent/testdata/zz_fixture_TestNewCreateCommand_When_flags_are_parsed_with_multi_label_selector__it_should_generate_correct_nodepool.yamlis excluded by!**/testdata/**product-cli/cmd/nodepool/agent/testdata/zz_fixture_TestNewCreateCommand_When_flags_are_parsed_with_single_label_selector__it_should_generate_correct_nodepool.yamlis excluded by!**/testdata/**product-cli/cmd/nodepool/aws/testdata/zz_fixture_TestNewCreateCommand_When_flags_are_parsed_with_full_configuration__it_should_generate_correct_nodepool.yamlis excluded by!**/testdata/**product-cli/cmd/nodepool/aws/testdata/zz_fixture_TestNewCreateCommand_When_flags_are_parsed_with_minimal_configuration__it_should_generate_correct_nodepool.yamlis excluded by!**/testdata/**product-cli/cmd/nodepool/azure/testdata/zz_fixture_TestNewCreateCommand_When_flags_are_parsed_with_full_configuration__it_should_generate_correct_nodepool.yamlis excluded by!**/testdata/**product-cli/cmd/nodepool/azure/testdata/zz_fixture_TestNewCreateCommand_When_flags_are_parsed_with_minimal_configuration__it_should_generate_correct_nodepool.yamlis excluded by!**/testdata/**product-cli/cmd/nodepool/kubevirt/testdata/zz_fixture_TestNewCreateCommand_When_flags_are_parsed_with_full_configuration__it_should_generate_correct_nodepool.yamlis excluded by!**/testdata/**product-cli/cmd/nodepool/kubevirt/testdata/zz_fixture_TestNewCreateCommand_When_flags_are_parsed_with_minimal_configuration__it_should_generate_correct_nodepool.yamlis excluded by!**/testdata/**product-cli/cmd/nodepool/openstack/testdata/zz_fixture_TestNewCreateCommand_When_flags_are_parsed_with_full_configuration_including_availability_zone__it_should_generate_correct_nodepool.yamlis excluded by!**/testdata/**product-cli/cmd/nodepool/openstack/testdata/zz_fixture_TestNewCreateCommand_When_flags_are_parsed_with_minimal_configuration__it_should_generate_correct_nodepool.yamlis excluded by!**/testdata/**
📒 Files selected for processing (5)
product-cli/cmd/nodepool/agent/create_test.goproduct-cli/cmd/nodepool/aws/create_test.goproduct-cli/cmd/nodepool/azure/create_test.goproduct-cli/cmd/nodepool/kubevirt/create_test.goproduct-cli/cmd/nodepool/openstack/create_test.go
260d2f2 to
185db27
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
product-cli/cmd/nodepool/aws/create_test.go (1)
140-154: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDefault-subnet fallback path is never exercised.
hclustersets a defaultsubnet-default, but both cases pass--subnet-idexplicitly, so the fallback-to-cluster-default logic inUpdateNodePoolisn't covered by these fixtures. Consider adding a case that omits--subnet-idto verify the default is picked up correctly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@product-cli/cmd/nodepool/aws/create_test.go` around lines 140 - 154, Add a test case for UpdateNodePool that omits the --subnet-id argument while using the existing hcluster fixture with subnet-default, and assert the node pool uses that cluster-default subnet. Keep the existing explicit-subnet cases unchanged.product-cli/cmd/nodepool/openstack/create_test.go (1)
89-133: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider a negative test for the required
flavorfield.
Validatereturns an error whenFlavoris empty, but no case here exercises that path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@product-cli/cmd/nodepool/openstack/create_test.go` around lines 89 - 133, Add a negative test case to the table-driven tests around openstackOpts.Validate that leaves the required Flavor option empty, and assert validation returns an error instead of proceeding to completion or fixture comparison. Reuse the existing test setup and identify the new case through the testCase structure.product-cli/cmd/nodepool/agent/create_test.go (1)
68-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest-case names don't follow the repo's "When ... it should ..." convention. In each file,
TestNewCreateCommandcorrectly uses "When ... it should ..." case names, but the siblingTestCreateNodePool_When_flags_are_parsed_it_should_generate_correct_nodepooltable uses plain descriptive names instead, the same root cause repeated across all four providers.
product-cli/cmd/nodepool/agent/create_test.go#L68-L79: rename "empty label selector" / "single label selector" / "multi label selector" to "When ... it should ..." form.product-cli/cmd/nodepool/aws/create_test.go#L82-L101: rename "minimal configuration" / "full configuration" to "When ... it should ..." form.product-cli/cmd/nodepool/azure/create_test.go#L103-L133: rename "minimal configuration" / "full configuration" to "When ... it should ..." form.product-cli/cmd/nodepool/openstack/create_test.go#L72-L87: rename "minimal configuration" / "full configuration with availability zone" to "When ... it should ..." form.As per coding guidelines: "Always use "When ... it should ..." format for describing test cases when creating unit tests".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@product-cli/cmd/nodepool/agent/create_test.go` around lines 68 - 79, Rename the table-driven test cases in TestCreateNodePool_When_flags_are_parsed_it_should_generate_correct_nodepool to follow the “When ... it should ...” convention: update the three selector cases in product-cli/cmd/nodepool/agent/create_test.go (lines 68-79), the minimal and full configuration cases in product-cli/cmd/nodepool/aws/create_test.go (lines 82-101), the corresponding cases in product-cli/cmd/nodepool/azure/create_test.go (lines 103-133), and the minimal and availability-zone cases in product-cli/cmd/nodepool/openstack/create_test.go (lines 72-87); change names only and preserve test behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@product-cli/cmd/nodepool/agent/create_test.go`:
- Around line 68-79: Rename the table-driven test cases in
TestCreateNodePool_When_flags_are_parsed_it_should_generate_correct_nodepool to
follow the “When ... it should ...” convention: update the three selector cases
in product-cli/cmd/nodepool/agent/create_test.go (lines 68-79), the minimal and
full configuration cases in product-cli/cmd/nodepool/aws/create_test.go (lines
82-101), the corresponding cases in
product-cli/cmd/nodepool/azure/create_test.go (lines 103-133), and the minimal
and availability-zone cases in product-cli/cmd/nodepool/openstack/create_test.go
(lines 72-87); change names only and preserve test behavior.
In `@product-cli/cmd/nodepool/aws/create_test.go`:
- Around line 140-154: Add a test case for UpdateNodePool that omits the
--subnet-id argument while using the existing hcluster fixture with
subnet-default, and assert the node pool uses that cluster-default subnet. Keep
the existing explicit-subnet cases unchanged.
In `@product-cli/cmd/nodepool/openstack/create_test.go`:
- Around line 89-133: Add a negative test case to the table-driven tests around
openstackOpts.Validate that leaves the required Flavor option empty, and assert
validation returns an error instead of proceeding to completion or fixture
comparison. Reuse the existing test setup and identify the new case through the
testCase structure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 4809255c-76e2-4a87-a0fd-f72a9ca36b71
⛔ Files ignored due to path filters (11)
product-cli/cmd/nodepool/agent/testdata/zz_fixture_TestCreateNodePool_When_flags_are_parsed_it_should_generate_correct_nodepool_empty_label_selector.yamlis excluded by!**/testdata/**product-cli/cmd/nodepool/agent/testdata/zz_fixture_TestCreateNodePool_When_flags_are_parsed_it_should_generate_correct_nodepool_multi_label_selector.yamlis excluded by!**/testdata/**product-cli/cmd/nodepool/agent/testdata/zz_fixture_TestCreateNodePool_When_flags_are_parsed_it_should_generate_correct_nodepool_single_label_selector.yamlis excluded by!**/testdata/**product-cli/cmd/nodepool/aws/testdata/zz_fixture_TestCreateNodePool_When_flags_are_parsed_it_should_generate_correct_nodepool_full_configuration.yamlis excluded by!**/testdata/**product-cli/cmd/nodepool/aws/testdata/zz_fixture_TestCreateNodePool_When_flags_are_parsed_it_should_generate_correct_nodepool_minimal_configuration.yamlis excluded by!**/testdata/**product-cli/cmd/nodepool/azure/testdata/zz_fixture_TestCreateNodePool_When_flags_are_parsed_it_should_generate_correct_nodepool_full_configuration.yamlis excluded by!**/testdata/**product-cli/cmd/nodepool/azure/testdata/zz_fixture_TestCreateNodePool_When_flags_are_parsed_it_should_generate_correct_nodepool_minimal_configuration.yamlis excluded by!**/testdata/**product-cli/cmd/nodepool/kubevirt/testdata/zz_fixture_TestCreateNodePool_When_flags_are_parsed_it_should_generate_correct_nodepool_full_configuration.yamlis excluded by!**/testdata/**product-cli/cmd/nodepool/kubevirt/testdata/zz_fixture_TestCreateNodePool_When_flags_are_parsed_it_should_generate_correct_nodepool_minimal_configuration.yamlis excluded by!**/testdata/**product-cli/cmd/nodepool/openstack/testdata/zz_fixture_TestCreateNodePool_When_flags_are_parsed_it_should_generate_correct_nodepool_full_configuration_with_availability_zone.yamlis excluded by!**/testdata/**product-cli/cmd/nodepool/openstack/testdata/zz_fixture_TestCreateNodePool_When_flags_are_parsed_it_should_generate_correct_nodepool_minimal_configuration.yamlis excluded by!**/testdata/**
📒 Files selected for processing (5)
product-cli/cmd/nodepool/agent/create_test.goproduct-cli/cmd/nodepool/aws/create_test.goproduct-cli/cmd/nodepool/azure/create_test.goproduct-cli/cmd/nodepool/kubevirt/create_test.goproduct-cli/cmd/nodepool/openstack/create_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- product-cli/cmd/nodepool/kubevirt/create_test.go
185db27 to
877b017
Compare
bryan-cox
left a comment
bryan-cox
left a comment
There was a problem hiding this comment.
Good coverage — all 5 platforms are present with Use field, flag registration, and flag default assertions matching the Jira acceptance criteria.
The fixture-driven UpdateNodePool pipeline tests go beyond the spec's scope (beneficial scope creep — they verify flags actually flow to the NodePool struct), but the ~30-line scaffold is nearly identical across all 5 files. If these tests are kept, consider extracting a shared test helper parameterized by platform to reduce the duplication.
877b017 to
9a0b20f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@product-cli/cmd/nodepool/openstack/create_test.go`:
- Around line 128-141: Update the test setup around UpdateNodePool to leave
nodePool.Spec.Platform.Type unset, then explicitly assert after the update that
it equals hyperv1.OpenStackPlatform. Keep the existing fixture comparison for
the OpenStack platform fields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 0ff4db90-14df-46d9-990a-c3a116d9e699
⛔ Files ignored due to path filters (12)
product-cli/cmd/nodepool/agent/testdata/zz_fixture_TestUpdateNodePool_When_empty_label_selector_is_provided__it_should_generate_correct_nodepool.yamlis excluded by!**/testdata/**product-cli/cmd/nodepool/agent/testdata/zz_fixture_TestUpdateNodePool_When_multi_label_selector_is_provided__it_should_generate_correct_nodepool.yamlis excluded by!**/testdata/**product-cli/cmd/nodepool/agent/testdata/zz_fixture_TestUpdateNodePool_When_single_label_selector_is_provided__it_should_generate_correct_nodepool.yamlis excluded by!**/testdata/**product-cli/cmd/nodepool/aws/testdata/zz_fixture_TestUpdateNodePool_When_full_configuration_is_provided__it_should_generate_correct_nodepool.yamlis excluded by!**/testdata/**product-cli/cmd/nodepool/aws/testdata/zz_fixture_TestUpdateNodePool_When_minimal_configuration_is_provided__it_should_generate_correct_nodepool.yamlis excluded by!**/testdata/**product-cli/cmd/nodepool/aws/testdata/zz_fixture_TestUpdateNodePool_When_subnet_id_is_omitted__it_should_use_the_hosted_cluster_default_subnet.yamlis excluded by!**/testdata/**product-cli/cmd/nodepool/azure/testdata/zz_fixture_TestUpdateNodePool_When_full_configuration_is_provided__it_should_generate_correct_nodepool.yamlis excluded by!**/testdata/**product-cli/cmd/nodepool/azure/testdata/zz_fixture_TestUpdateNodePool_When_minimal_configuration_is_provided__it_should_generate_correct_nodepool.yamlis excluded by!**/testdata/**product-cli/cmd/nodepool/kubevirt/testdata/zz_fixture_TestUpdateNodePool_When_full_configuration_is_provided__it_should_generate_correct_nodepool.yamlis excluded by!**/testdata/**product-cli/cmd/nodepool/kubevirt/testdata/zz_fixture_TestUpdateNodePool_When_minimal_configuration_is_provided__it_should_generate_correct_nodepool.yamlis excluded by!**/testdata/**product-cli/cmd/nodepool/openstack/testdata/zz_fixture_TestUpdateNodePool_When_full_configuration_with_availability_zone_is_provided__it_should_generate_correct_nodepool.yamlis excluded by!**/testdata/**product-cli/cmd/nodepool/openstack/testdata/zz_fixture_TestUpdateNodePool_When_minimal_configuration_is_provided__it_should_generate_correct_nodepool.yamlis excluded by!**/testdata/**
📒 Files selected for processing (5)
product-cli/cmd/nodepool/agent/create_test.goproduct-cli/cmd/nodepool/aws/create_test.goproduct-cli/cmd/nodepool/azure/create_test.goproduct-cli/cmd/nodepool/kubevirt/create_test.goproduct-cli/cmd/nodepool/openstack/create_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- product-cli/cmd/nodepool/agent/create_test.go
- product-cli/cmd/nodepool/aws/create_test.go
- product-cli/cmd/nodepool/azure/create_test.go
- product-cli/cmd/nodepool/kubevirt/create_test.go
None of the 5 platform - Agent, AWS, Azure, KubeVirt and OpenStack had nodepool create commands. Signed-off-by: mehabhalodiya <mehabhalodiya@gmail.com>
9a0b20f to
e63d35b
Compare
|
@mehabhalodiya: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox, mehabhalodiya 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 |
openshift-ci
Bot
added
the
approved
|
/verified by unit tests |
openshift-ci-robot
added
the
verified
|
@mehabhalodiya: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
jparrill
left a comment
jparrill
left a comment
There was a problem hiding this comment.
Dropped some comments. Thanks!
| }, | ||
| } | ||
|
|
||
| if err := platformOpts.UpdateNodePool(ctx, nodePool, nil, nil); err != nil { |
There was a problem hiding this comment.
nit: Agent and AWS use the raw string "amd64" while Azure, KubeVirt and OpenStack use string(hyperv1.ArchitectureAMD64). Could you align these to use the typed constant for consistency? If the constant value ever changes, the literal would silently test the wrong arch.
| Spec: hyperv1.NodePoolSpec{ | ||
| Arch: "amd64", | ||
| Platform: hyperv1.NodePoolPlatform{ | ||
| Type: hyperv1.AgentPlatform, |
There was a problem hiding this comment.
I notice the Agent TestUpdateNodePool constructs options differently from the other 4 platforms — it creates a throwaway &cobra.Command{} and manually copies the flag value, while the others use DefaultOptions() → BindFlags() → Parse() → Validate() → Complete(). Is this because the Agent platform doesn't expose that pipeline yet? If so, could you add a brief comment explaining the difference? As-is, if a new flag is added to the Agent platform, the manual copy step could be silently missed.
| Arch: string(hyperv1.ArchitectureAMD64), | ||
| } | ||
| openstackOpts := openstacknodepool.DefaultOptions() | ||
| openstacknodepool.BindOptions(openstackOpts, flags) |
There was a problem hiding this comment.
Minor: all other platforms pre-set Platform.Type on the nodePool struct, but OpenStack omits it and then asserts it after UpdateNodePool. Is this intentional because OpenStack's UpdateNodePool sets the type itself? If so, a quick comment would help future readers understand the difference isn't a copy-paste oversight.
5 participants
None of the 5 platforms - Agent, AWS, Azure, KubeVirt, and OpenStack had nodepool create commands.
What this PR does / why we need it:
gp3), root-volume-size default (120), exact 8-flag enumeration120), disk-storage-account-type default (Premium_LRS), exact 15-flag enumeration4Gi), cores default (2), root-volume-size default (32), qos-class default (Burstable), network-multiqueue default (Enable), attach-default-network default (true), developer-only flag exclusion (containerdisk), exact 13-flag enumerationWhich issue(s) this PR fixes:
Fixes https://redhat.atlassian.net/browse/CNTRLPLANE-3897
Special notes for your reviewer:
Checklist:
Summary by CodeRabbit