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.

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

Fix: Allow modifying NFS Export with valid zero values #186

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rounak-adhikary
Copy link
Contributor

@rounak-adhikary rounak-adhikary commented Mar 11, 2025

PR Submission checklist

GitHub Issues

List the GitHub issues impacted by this PR:

GitHub Issue #
dell/terraform-provider-powerstore#120

Common PR Checklist:

  • Have you made sure that the code compiles?
  • Have you commented your code, particularly in hard-to-understand areas?
  • Did you run tests in a real Kubernetes cluster?
  • Have you maintained backward compatibility?
  • Have you updated the mocks for any Client functions that have been modified (mocks/Client.go)?

Description of your changes:

Allowing patching NFS Export with valid zero values.
The JSON tag for NFS Owner Username field. Fixed that.

This fix is similar to #184

Tested via

Integration Tests

root@lglap049:~/gopowerstore# go test -run "NFSExport" ./inttests
ok      github.com/dell/gopowerstore/inttests   84.368s
root@lglap049:~/gopowerstore#

@rounak-adhikary rounak-adhikary changed the title Allowing patching NFS Export with valid zero values Fix: Allow modifying NFS Export with valid zero values Mar 11, 2025
Copy link

Merging this branch will not change overall coverage

Impacted Packages Coverage Δ 🤖
github.com/dell/gopowerstore 91.63% (ø)

Coverage by file

Changed files (no unit tests)

Changed File Coverage Δ Total Covered Missed 🤖
github.com/dell/gopowerstore/nfs_types.go 100.00% (ø) 2 2 0

Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code.

@@ -103,23 +104,30 @@ type NFSExportModify struct {
// NFS enforced security type for users accessing an NFS Export.
MinSecurity string `json:"min_security,omitempty"`
// Hosts with no access to the NFS export or its snapshots.
NoAccessHosts []string `json:"no_access_hosts,omitempty"`
// Empty list is a valid value, so omitempty should not be used.
NoAccessHosts []string `json:"no_access_hosts"`
Copy link
Contributor

@santhoshatdell santhoshatdell Mar 11, 2025

Choose a reason for hiding this comment

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

Hi,
For all the hosts list, there are 'add' and 'remove' lists in NFSExportModify. Could you check if you can make use of those fields to add or remove a host from the list during modify operation.?
Removing omitempty for these lists have an effect on other Client library users.

// Specifies the user ID of the anonymous account.
AnonymousUID int32 `json:"anonymous_UID,omitempty"`
// Zero ID is a valid value, so omitempty should not be used.
AnonymousUID int32 `json:"anonymous_UID"`
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure if one can unset AnonymousUID and GID. If that is not allowed, let's not remove 'omitempty'.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants