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

NFS Export operations support for NFS for powerflex v4.0 #55

Merged
merged 11 commits into from
Apr 21, 2023

Conversation

KshitijaKakde
Copy link
Contributor

@KshitijaKakde KshitijaKakde commented Apr 18, 2023

Description

This PR includes changes for adding operation for NFS export for PowerFlex storage system v4.0

GitHub Issues

List the GitHub issues impacted by this PR:

GitHub Issue #
dell/csm#763

Checklist:

  • I have performed a self-review of my own code to ensure there are no formatting, vetting, linting, or security issues
  • I have verified that new and existing unit tests pass locally with my changes
  • I have not allowed coverage numbers to degenerate
  • I have maintained at least 90% code coverage
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • Backward compatibility is not broken

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Please also list any relevant details for your

Integration Test:
inti

Unit Test

2

3

4

1

@KshitijaKakde KshitijaKakde marked this pull request as ready for review April 18, 2023 15:19
Copy link
Contributor

@khareRajshree khareRajshree left a comment

Choose a reason for hiding this comment

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

@KshitijaKakde, please rebase this branch with main as I see a couple of changes not reflexting here since the last merge.

Also fix the linting errors reported from github workflow.

Copy link
Contributor

@khareRajshree khareRajshree left a comment

Choose a reason for hiding this comment

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

@KshitijaKakde Please address the below review comments. Thanks.

if nfsexport == nil {
return ""
}
fmt.Printf("filesystems[0].Name: %v", nfsexport[0].Name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this print statement from integrations test.

return nfsexport[0].Name
}

// func TestGetFileSystemByName(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the commented code.

// }

func TestNFSExportByName(t *testing.T) {
// system := getSystem()
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the commented code.

assert.NotNil(t, system)

nfsName := fmt.Sprintf("%s-%s", "NFS", testPrefix+randString(8))
nfsmodify := "NFS export modify testing"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this print statement from integrations test.

nfs_export.go Outdated
}

return respnfs, nil

Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this new line..

@@ -14,6 +14,10 @@ GOSCALEIO_INSTALLATIONID=08bhsd7f9cfy890g
GOSCALEIO_NASSERVER=env8nasserver
GOSCALEIO_FILESYSTEM=env8FS

# For NFSExport operations , enable these values
GOSCALEIO_NFSEXPORT=share1
GOSCALEIO_FILESYSTEM_NFSEXPORT=twee-fs11
Copy link
Contributor

Choose a reason for hiding this comment

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

please put a generic dummy value for GOSCALEIO_FILESYSTEM_NFSEXPORT

Copy link
Contributor

@khareRajshree khareRajshree left a comment

Choose a reason for hiding this comment

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

for the UTs and in-tests, please update with the latest results covering for all methods.

Copy link
Contributor

@VamsiSiddu-7 VamsiSiddu-7 left a comment

Choose a reason for hiding this comment

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

@KshitijaKakde I have added some review comments. Please address those.

}

// CreateResponse defines struct for response
type CreateResponse struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

can you rename this to NFSExportCreateResponse

// NFSExport defines the struct for NFSExport
type NFSExport struct {
ID string
FileSystemID string `json:"file_system_id,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

if you dont provide the json: tag it cannot parse the id.

@KshitijaKakde KshitijaKakde merged commit c4e7883 into main Apr 21, 2023
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.

4 participants