Skip to content

Volume Group Snapshot Modify Fix#124

Merged
prajwalpatil25 merged 6 commits into
mainfrom
volume-group-snapshot-modify-fix
Feb 15, 2024
Merged

Volume Group Snapshot Modify Fix#124
prajwalpatil25 merged 6 commits into
mainfrom
volume-group-snapshot-modify-fix

Conversation

@doriac11

@doriac11 doriac11 commented Feb 13, 2024

Copy link
Copy Markdown
Contributor

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

Description of your changes:

  • When modifing a volume group snapshot, if the protection policy id field is set it will cause the API to fail. Created a separate method for VolumeGroupSnapshotModify with a different body removing the protection policy id field.

Test Output for new tests

image

Coverage of new method (100%)

image

@doriac11

Copy link
Copy Markdown
Contributor Author

I ran make gosec locally and there were no issues, not sure why that step was failing.

@adarsh-dell

Copy link
Copy Markdown
Contributor

Hi @doriac11 , Will it be possible to add the mock and UT for this new method?

- When modifing a volume group snapshot, if the protection policy id
  field is set it will cause the API to fail. Created a seperate method
  for VGsnapshotModify with a different body.
@doriac11 doriac11 force-pushed the volume-group-snapshot-modify-fix branch from bbb6c09 to 72ef03e Compare February 14, 2024 13:07
@doriac11

Copy link
Copy Markdown
Contributor Author

Hi @doriac11 , Will it be possible to add the mock and UT for this new method?

Added the UTs and Mock Data, screenshots in the description.

@AkshaySainiDell AkshaySainiDell left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

shekhar-j
shekhar-j previously approved these changes Feb 14, 2024
nikitajoshi1
nikitajoshi1 previously approved these changes Feb 14, 2024
@adarsh-dell

Copy link
Copy Markdown
Contributor

Hi @doriac11 , Will it be possible to add the mock and UT for this new method?

Added the UTs and Mock Data, screenshots in the description.

Thanks @doriac11

adarsh-dell
adarsh-dell previously approved these changes Feb 15, 2024

@adarsh-dell adarsh-dell left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM.

@delldubey delldubey force-pushed the volume-group-snapshot-modify-fix branch from 12ec759 to f05c786 Compare February 15, 2024 10:51
@delldubey delldubey force-pushed the volume-group-snapshot-modify-fix branch from f05c786 to 14b1c64 Compare February 15, 2024 11:09
@delldubey delldubey force-pushed the volume-group-snapshot-modify-fix branch from 14b1c64 to abbef3c Compare February 15, 2024 11:25
@prajwalpatil25 prajwalpatil25 merged commit 38e5ba4 into main Feb 15, 2024
@shanmydell shanmydell deleted the volume-group-snapshot-modify-fix branch February 15, 2024 12:15
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.

7 participants