-
Notifications
You must be signed in to change notification settings - Fork 44
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
[Module][1717]new_module_zfs_resize #1767
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not have thought to make this into an object. Nicely done!
) | ||
else: | ||
if verbose: | ||
os.remove(tmp_file) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know tmp_file is being removed inside get_full_output, I would recommend deleting the tmp_file in this flow so other people reading the code will know for sure, instead of checking that other function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making all those changes, looks great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work on this, thank you for addressing my multiple comments promptly and dealing with my changes of mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AndreMarcel99 , this looks great, despite my request for changes, you did a really great job cleaning up this module and making it yours.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the quick turn around and addressing all my comments.
What is pending is a discussion with the team on using target
vs src
and no_auto_increment
vs no_auto_increase
and then the idea that we inspect the data set for a trace to make sure it meets the minimum requirements. I would be ok if we took the last comment (the minimum requirements as a new work item for the backlog as well).
Signed-off-by: ddimatos <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing all the comments, I have one more, I missed this in my last review. Let me know if you think otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks for all the hard work and timely updates.
SUMMARY
First draft on support zfs resizing module
Fixes #1717
ISSUE TYPE
COMPONENT NAME
ADDITIONAL INFORMATION
### module_util/zfsadm
As the aggregate_name is common for activities using command zfsadm, reason to use in the init function and the module to execute any command, the class is expecting to grow for other modules.
Function grow_shrink
Execute the principal function for the module resize, waiting on the confirmation of operation and the extra instructions.
### modules/zfs_resize
Function found_mount_target
Function to validate if the zfs_dataset exist and get the USS mount_point for the response
Function get_size_and_free parsing function for the response of free and use size of the zfs dataset
Function create_command function to set extra information to execute the resize
output
I know this is not the best way to show pass the test case but related to issue the parallel testing for no auto increment is failing on the pipeline