Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Enhancement]: Add interface service_policy parameter #332

Merged
merged 7 commits into from
Feb 6, 2025
Merged

[Enhancement]: Add interface service_policy parameter #332

merged 7 commits into from
Feb 6, 2025
+111 −36

Conversation

acch
Copy link
Contributor

@acch acch commented Oct 31, 2024 •
edited
Loading

Adds ability to configure service_policy parameter of IP Interfaces.

Closes #306.

Acceptance tests pass:

$ TF_ACC=1 go test ./internal/provider/networking/network_ip_interface_resource_test.go -v
=== RUN   TestAccNetworkIpInterfaceResource
--- PASS: TestAccNetworkIpInterfaceResource (4.44s)
PASS
ok      command-line-arguments  4.449s
$ TF_ACC=1 go test ./internal/provider/networking/network_ip_interface_resource_alias_test.go -v
=== RUN   TestAccNetworkIpInterfaceResourceAlias
--- PASS: TestAccNetworkIpInterfaceResourceAlias (3.93s)
PASS
ok      command-line-arguments  3.935s

Example Terraform Configuration:

// sample IP Interface with service_policy parameter
resource "netapp-ontap_networking_ip_interface" "example" {
  cx_profile_name = ...
  name            = ...
  svm_name        = ...
  ...
  service_policy   = "default-management"
}

@acch acch force-pushed the 306-interface-service_policy-parameter branch 2 times, most recently from 71823b2 to 1de03df Compare October 31, 2024 12:40
@acch acch marked this pull request as ready for review October 31, 2024 13:16
@acch acch mentioned this pull request Oct 31, 2024
Closed
@carchi8py carchi8py added the enhancement New feature or request label Oct 31, 2024
@carchi8py carchi8py added this to the 2.1 milestone Oct 31, 2024
@acch acch force-pushed the 306-interface-service_policy-parameter branch from d2aa5ab to 2552062 Compare November 4, 2024 09:17
@acch acch force-pushed the 306-interface-service_policy-parameter branch from 39b39bc to dd0999b Compare December 9, 2024 14:23
chuyich
chuyich requested changes Jan 2, 2025
View reviewed changes
Copy link
Contributor

@chuyich chuyich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In resource, the field service_policy should not be set default value at the beginning. Based on the API (9.6) document, this field does not have to be set in the POST API call. Please make change accordingly.

carchi8py
carchi8py requested changes Jan 6, 2025
View reviewed changes
Copy link
Contributor

@carchi8py carchi8py left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing updates to the documentation pages for the new options.

@acch acch force-pushed the 306-interface-service_policy-parameter branch from dd0999b to 57fc48c Compare January 7, 2025 10:40
@acch
Copy link
Contributor Author

acch commented Jan 7, 2025

Thanks @carchi8py and @chuyich for your feedback!

I've removed the default value for service_policy, and added logic for fetching the value computed by ONTAP API instead. I've also updated the documentation for the new parameter, and verified that acceptance tests (still) pass.
Please let me know if this is acceptable, or what is missing.

Thanks much!

@acch acch requested review from carchi8py and chuyich January 13, 2025 10:23
@acch acch force-pushed the 306-interface-service_policy-parameter branch from 52ad1a4 to 7b16470 Compare January 22, 2025 13:17
@acch
Copy link
Contributor Author

acch commented Feb 1, 2025

@carchi8py Are we good to merge this PR?

Please let me know what I need to do exactly to have the PR added to the provider. My understanding is that I've addressed all requested changes... please let me know what's missing.

Many thanks!

@suhasbshekar
Copy link
Contributor

suhasbshekar commented Feb 3, 2025 •
edited
Loading

Can you update CHANGELOG.md file?

Also your branch is behind the integration/main, we added few more checks to the pipeline as it was missing here, since your PR is from a fork of this repo, so could you pull the new changes into your branch and update the PR?

Just for our reference, since we have disabled ACC tests as our testing profiles are down, acc tests are passing for this PR when tested locally.

@acch acch force-pushed the 306-interface-service_policy-parameter branch from 7b16470 to a6da4e5 Compare February 4, 2025 08:38
@acch
Copy link
Contributor Author

acch commented Feb 4, 2025

@suhasbshekar Thanks for your feedback.

  • I've added an entry to CHANGELOG.md
  • I've rebased my branch to integration/main (once again)

Acc tests are passing on my local test system (ONTAP Select running 9.15.1P4):

$ date && TF_ACC=1 go test ./internal/provider/networking/network_ip_interface_resource_test.go -v && date
Tue Feb  4 08:48:40 UTC 2025
=== RUN   TestAccNetworkIpInterfaceResource
--- PASS: TestAccNetworkIpInterfaceResource (4.22s)
PASS
ok      command-line-arguments  4.233s
Tue Feb  4 08:48:45 UTC 2025

Please let me know what else is needed.
Many thanks!

@suhasbshekar suhasbshekar removed this from the 2.1 milestone Feb 5, 2025
@suhasbshekar
Copy link
Contributor

Please resolve the conflicts.

@acch acch force-pushed the 306-interface-service_policy-parameter branch from a6da4e5 to 3f33bff Compare February 6, 2025 19:01
@acch
Copy link
Contributor Author

acch commented Feb 6, 2025

@suhasbshekar I've rebased the PR once again.

Can you please let me know when you'll be able to merge these changes?

@suhasbshekar
Copy link
Contributor

@acch I have merged one PR, please keep an eye on your PRs, as there will be conflicts once any PR is merged. For now please resolve the conflicts of this PR

acch added 7 commits February 6, 2025 20:09
@acch
feat: add service_policy parameter to ip_interface(s) data source
93480c6 
Signed-off-by: Achim Christ <achim.christ@sva.de>
@acch
feat: add service_policy parameter to ip_interface resource
c4104da 
Signed-off-by: Achim Christ <achim.christ@sva.de>
@acch
feat: add acceptance tests for ip_interface resource service_policy…
31c5c60 
… parameter

Signed-off-by: Achim Christ <achim.christ@sva.de>
@acch
fix: remove duplicate imports
8c698de 
Signed-off-by: Achim Christ <achim.christ@sva.de>
@acch
fix: remove default value for service_policyparameter
6899220 
Signed-off-by: Achim Christ <achim.christ@sva.de>
@acch
chore: add service_policy parameter to attribute list
d8864c9 
Signed-off-by: Achim Christ <achim.christ@sva.de>
@acch
chore: update changelog
69471ca 
Signed-off-by: Achim Christ <achim.christ@sva.de>
@acch acch force-pushed the 306-interface-service_policy-parameter branch from 3f33bff to 69471ca Compare February 6, 2025 20:09
@acch
Copy link
Contributor Author

acch commented Feb 6, 2025

@suhasbshekar Sure, resolved the conflict in this PR.

@suhasbshekar suhasbshekar merged commit 19ec8d2 into NetApp:integration/main Feb 6, 2025
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@suhasbshekar suhasbshekar

@wenjun666 wenjun666

@carchi8py carchi8py

@chuyich chuyich

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Enhancement]: Add interface service_policy parameter
4 participants
@acch @suhasbshekar @carchi8py @chuyich