feat: add initiators field to create_igroup for atomic create-with-membership#178
Open
dbtinsley wants to merge 3 commits into
Open
feat: add initiators field to create_igroup for atomic create-with-membership#178dbtinsley wants to merge 3 commits into
dbtinsley wants to merge 3 commits into
Conversation
dbtinsley
requested review from
Hardikl,
cgrinds and
rahulguptajss
as code owners
July 13, 2026 16:57
|
Thank you for your interest in contributing to the ontap-mcp project! We require contributors to sign our Corporate contributor license agreement (CCLA), and we don"t have the user(s) @dbtinsley on file. In order for us to review and merge your code, please follow the instructions in step 6 of creating a pull request. |
There was a problem hiding this comment.
Pull request overview
This PR extends the typed igroup creation flow to support atomic “create igroup with initiators” in a single ONTAP REST request, and refactors REST job handling to correctly poll only for async (202 Accepted) responses.
Changes:
- Add optional
initiatorsinput totool.IGroupCreate, plumbed throughserver.newCreateIGroupintoontap.IGroup.Initiators. - Refactor
rest.Client.handleJobto return immediately for synchronous success and poll only for 202 responses; introducejobPollIntervalfor test isolation. - Update multiple REST mutation calls to use
handleJoband add unit tests covering job polling and new igroup initiator behavior.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tool/tool.go | Adds Initiators []string to the typed create_igroup input schema. |
| ontap/ontap.go | Adds Initiators []InitiatorName to the ONTAP igroup request/response model. |
| server/igroup.go | Maps initiators into the create body and adds protocol validation/guard logic. |
| server/igroup_typed_fields_test.go | Adds unit tests for initiator mapping and NVMe protocol rejection behavior. |
| rest/client.go | Refactors handleJob and adds configurable polling interval. |
| rest/igroup.go | Captures response body and routes create through handleJob. |
| rest/lun.go | Captures response body and routes create through handleJob. |
| rest/lunmap.go | Captures response body and routes create through handleJob. |
| rest/nvme.go | Captures response body and routes NVMe creates through handleJob. |
| rest/qospolicy.go | Captures response body and routes QoS create/delete through handleJob. |
| rest/job_wait_test.go | Adds tests validating handleJob behavior and polling across common mutations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+165
to
+170
| if in.Protocol == "" { | ||
| return out, errors.New("protocol is required") | ||
| } | ||
| if strings.EqualFold(in.Protocol, "nvme") { | ||
| return out, errors.New("NVMe initiators cannot be created with an igroup; use an NVMe subsystem and NQN hosts") | ||
| } |
Comment on lines
+179
to
+184
| for _, initiator := range in.Initiators { | ||
| if initiator == "" { | ||
| return out, errors.New("all initiators must be non-empty") | ||
| } | ||
| out.Initiators = append(out.Initiators, ontap.InitiatorName{Name: initiator}) | ||
| } |
Comment on lines
+23
to
+30
| body, err := json.Marshal(got) | ||
| if err != nil { | ||
| t.Fatalf("json.Marshal() error = %v", err) | ||
| } | ||
| want := `{"svm":{"name":"genio"},"name":"genio-fcp","os_type":"linux","protocol":"fcp","initiators":[{"name":"10:00:00:00:00:00:00:01"},{"name":"10:00:00:00:00:00:00:02"}]}` | ||
| if string(body) != want { | ||
| t.Errorf("igroup create body = %s, want %s", body, want) | ||
| } |
dbtinsley
added a commit
to dbtinsley/ontap-mcp
that referenced
this pull request
Jul 13, 2026
- NVMe guard is now conditional: provides a distinct error message when initiators are supplied (directing to NVMe subsystem + NQN hosts) versus when the nvme protocol is used without initiators (general protocol error). - Initiator validation now trims whitespace before the empty check, rejecting values like " " that would cause confusing ONTAP API errors downstream. - Test uses struct comparison via json.Marshal(want) instead of a raw string literal; adds coverage for NVMe-without-initiators and whitespace-only cases. Addresses review comments on NetApp#178. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Thank you for your interest in contributing to the ontap-mcp project! We require contributors to sign our Corporate contributor license agreement (CCLA), and we don"t have the user(s) @dbtinsley on file. In order for us to review and merge your code, please follow the instructions in step 6 of creating a pull request. |
Author
|
@cla-bot check Note for the ontap-mcp team: @dbtinsley is a NetApp employee contributing this work as part of a NetApp-sponsored project (NetApp CPOC). We believe the internal employment agreement covers IP assignment and that the external CCLA process does not apply, but please let us know if there is an internal process we should follow instead. |
|
Thank you for your interest in contributing to the ontap-mcp project! We require contributors to sign our Corporate contributor license agreement (CCLA), and we don"t have the user(s) @dbtinsley on file. In order for us to review and merge your code, please follow the instructions in step 6 of creating a pull request. |
|
The cla-bot has been summoned, and re-checked this pull request! |
| if req.Method != http.MethodGet { | ||
| t.Fatalf("job poll method = %s, want GET", req.Method) | ||
| } | ||
| *jobCalls++ |
Comment on lines
+165
to
172
| switch { | ||
| case in.Protocol == "": | ||
| return out, errors.New("protocol is required; valid values are fcp, iscsi, mixed") | ||
| case strings.EqualFold(in.Protocol, "nvme") && len(in.Initiators) > 0: | ||
| return out, errors.New("NVMe igroups do not support FC/iSCSI initiators; use an NVMe subsystem with NQN hosts instead") | ||
| case strings.EqualFold(in.Protocol, "nvme"): | ||
| return out, errors.New("nvme is not a valid igroup protocol; use an NVMe subsystem instead (fcp, iscsi, or mixed are valid igroup protocols)") | ||
| } |
Comment on lines
+181
to
+186
| for _, initiator := range in.Initiators { | ||
| if strings.TrimSpace(initiator) == "" { | ||
| return out, errors.New("all initiators must be non-empty and non-whitespace") | ||
| } | ||
| out.Initiators = append(out.Initiators, ontap.InitiatorName{Name: initiator}) | ||
| } |
dbtinsley
added a commit
to dbtinsley/ontap-mcp
that referenced
this pull request
Jul 13, 2026
… before appending
- Validates protocol against the allowed set {fcp, iscsi, mixed} and returns
a descriptive error for anything else, preventing unsupported values from
reaching the ONTAP API with a misleading validation message.
- Trims whitespace from each initiator before appending to the request body
so the validation check and the payload are consistent; a caller passing
' wwpn ' now gets the trimmed value sent to ONTAP rather than the raw
string.
Addresses second-round review comments on NetApp#178.
Co-authored-by: Cursor <cursoragent@cursor.com>
Comment on lines
+173
to
+174
| case !validProtocols[strings.ToLower(in.Protocol)]: | ||
| return out, fmt.Errorf("unsupported igroup protocol %q; valid values are fcp, iscsi, mixed", in.Protocol) |
dbtinsley
added a commit
to dbtinsley/ontap-mcp
that referenced
this pull request
Jul 13, 2026
… body Case-insensitive validation accepted inputs like "FCP" but stored the original casing verbatim, which may be rejected by some ONTAP REST endpoints. Normalizing to lowercase ensures the stored protocol is canonical regardless of caller casing. Addresses review comment on NetApp#178. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Thank you for your interest in contributing to the ontap-mcp project! We require contributors to sign our Corporate contributor license agreement (CCLA), and we don"t have the user(s) @dbtinsley on file. In order for us to review and merge your code, please follow the instructions in step 6 of creating a pull request. |
Hardikl
reviewed
Jul 15, 2026
| } | ||
| if in.Protocol == "" { | ||
| return out, errors.New("protocol is required") | ||
| validProtocols := map[string]bool{"fcp": true, "iscsi": true, "mixed": true} |
Contributor
There was a problem hiding this comment.
tools.go says this protocol field has only 3 type. LLM would handle this validation.
Still, explicit validation required, then you move logic !validProtocols[strings.ToLower(in.Protocol)] at top, which remove nvme cases as any protocol value other than 3 are invalid.
| @@ -0,0 +1,106 @@ | |||
| package server | |||
Contributor
There was a problem hiding this comment.
use this test file for new testcase: https://github.com/NetApp/ontap-mcp/blob/main/integration/test/igroup_test.go
…mbership Allows specifying FC/iSCSI initiators at igroup creation time so a single tool call creates the igroup and populates its membership atomically, rather than requiring a separate add_igroup_initiator call per initiator. Changes: - tool.IGroupCreate gains Initiators []string field - ontap.IGroup gains Initiators []InitiatorName for REST serialization - newCreateIGroup validates each initiator (trimmed, non-empty), normalizes protocol to lowercase, and validates against the fcp/iscsi/mixed whitelist (NVMe rejected as unsupported protocol, no longer a special case) - rest.CreateIGroup upgraded from checkStatus to handleJob so ONTAP 202 async responses are polled to completion; same fix applied to CreateLUN, CreateLunMap, and CreateNVMeNamespace which can also return 202 - rest.Client.handleJob refactored: only polls when status is 202 Accepted (not 201 Created), guards against empty job UUID — fixes a bug in main where 201 responses incorrectly attempted job polling - integration/test/igroup_test.go: existing "Create igroup" test replaced with "Create igroup with initiators atomically" that verifies membership immediately after create; standalone server/igroup_typed_fields_test.go and rest/job_wait_test.go removed per maintainer guidance Co-authored-by: Cursor <cursoragent@cursor.com>
dbtinsley
force-pushed
the
cpoc/feat/igroup-initiators
branch
from
July 15, 2026 16:15
799968e to
2254e51
Compare
Author
|
Round 2 update — rebased on main (post-#157), addressed all maintainer and Copilot feedback: Shared changes across all four PRs:
PR-specific changes:
|
Comment on lines
3
to
7
| import ( | ||
| "bytes" | ||
| "context" | ||
| "fmt" | ||
| "github.com/netapp/ontap-mcp/ontap" |
Comment on lines
3
to
7
| import ( | ||
| "bytes" | ||
| "context" | ||
| "fmt" | ||
| "github.com/netapp/ontap-mcp/ontap" |
Comment on lines
+219
to
+223
| validProtocols := map[string]bool{"fcp": true, "iscsi": true, "mixed": true} | ||
| switch { | ||
| case in.Protocol == "": | ||
| return out, errors.New("protocol is required; valid values are fcp, iscsi, mixed") | ||
| case !validProtocols[strings.ToLower(in.Protocol)]: |
Comment on lines
+44
to
+48
| func (c *Client) handleJob(ctx context.Context, statusCode int, buf bytes.Buffer) error { | ||
| if statusCode == http.StatusCreated || statusCode == http.StatusAccepted { | ||
| var pj ontap.PostJob | ||
| err := json.Unmarshal(buf.Bytes(), &pj) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if err := c.checkStatus(statusCode); err != nil { | ||
| return err | ||
| } | ||
| if statusCode != http.StatusAccepted { |
…files - Change handleJob signature to accept *bytes.Buffer to avoid copying a potentially large buffer by value; update all call sites in rest/ - Group third-party imports separately in rest/lun.go and rest/nvme.go to satisfy goimports ordering requirements Co-authored-by: Cursor <cursoragent@cursor.com>
Author
Round 3 update — igroup-initiators (#178)Changes pushed in this update ( REST / infra
No behaviour changes — this is purely mechanical cleanup addressing static-analysis and formatting feedback. |
Comment on lines
+219
to
+223
| validProtocols := map[string]bool{"fcp": true, "iscsi": true, "mixed": true} | ||
| switch { | ||
| case in.Protocol == "": | ||
| return out, errors.New("protocol is required; valid values are fcp, iscsi, mixed") | ||
| case !validProtocols[strings.ToLower(in.Protocol)]: |
Comment on lines
28
to
35
| type Client struct { | ||
| poller *config.Poller | ||
| httpClient *http.Client | ||
| credCache credentialsCache | ||
| remote ontap.Remote | ||
| initOnce sync.Once | ||
| poller *config.Poller | ||
| httpClient *http.Client | ||
| credCache credentialsCache | ||
| remote ontap.Remote | ||
| initOnce sync.Once | ||
| jobPollInterval time.Duration | ||
| } |
…ation Ensure leading/trailing whitespace and case variations like ' FCP ' are normalized to their canonical form before the empty check and the whitelist map lookup, keeping the error message accurate (it echoes the original caller-supplied value). Co-authored-by: Cursor <cursoragent@cursor.com>
Author
Round 4 update — igroup-initiators (#178)Addressing latest Copilot feedback (commit Fixed
Stale / already addressed in round 3
By design (no change)
|
Comment on lines
+219
to
226
| protocol := strings.ToLower(strings.TrimSpace(in.Protocol)) | ||
| validProtocols := map[string]bool{"fcp": true, "iscsi": true, "mixed": true} | ||
| switch { | ||
| case protocol == "": | ||
| return out, errors.New("protocol is required; valid values are fcp, iscsi, mixed") | ||
| case !validProtocols[protocol]: | ||
| return out, fmt.Errorf("unsupported igroup protocol %q; valid values are fcp, iscsi, mixed", in.Protocol) | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #173
What this PR does
Adds an optional
initiators []stringfield toIGroupCreateso callers can supply the complete FC WWPN / iSCSI IQN / EUI initiator list in a singlePOST /protocols/san/igroups, matching the ONTAP REST API's native capability.Creating an igroup and then adding initiators one at a time is not atomic: a partial failure leaves an empty igroup on live storage. This field eliminates that window.
An explicit guard rejects
protocol=nvmewith an initiator list and directs callers to use an NVMe subsystem with NQN hosts instead.Supporting change:
rest.Client.handleJobrefactorThis PR also refactors
handleJobto correctly distinguish 202 Accepted (async, requires job polling) from 201 Created (sync, no polling). Previously,handleJobattempted job body parsing for both status codes. Now it callscheckStatusfirst, returns immediately on 201, and only polls the job queue for 202. AjobPollIntervalfield is added toClientfor test isolation.CreateIGroup,CreateLUN,CreateLunMap,CreateNVMeNamespace,CreateNVMeSubsystemMap, andCreateQoSPolicy/DeleteQoSPolicyare updated to usehandleJob(with response body capture) so they handle ONTAP async job responses correctly. The samehandleJobchange appears in the companion PRs (#174, #175, #176) and only needs to be merged once.Files changed
tool/tool.goInitiators []stringtoIGroupCreateontap/ontap.goInitiators []InitiatorNametoIGroupserver/igroup.gonewCreateIGroup; add NVMe protocol guardserver/igroup_typed_fields_test.gorest/client.gojobPollInterval; refactorhandleJobrest/igroup.go+lun.go+lunmap.go+nvme.go+qospolicy.gohandleJobinstead ofcheckStatusrest/job_wait_test.gohandleJobunit tests and mutation job-polling testsRaised by NetApp CPOC.
Made with Cursor