-
Notifications
You must be signed in to change notification settings - Fork 95
Add VSAN policy update code to admin api #450
Conversation
@@ -151,6 +151,21 @@ def commands(): | |||
'func': policy_ls, | |||
'help': | |||
'List storage policies and volumes using those policies' | |||
}, | |||
'update': { |
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.
Nice - and easy to review / read :-)
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.
haha :D
All in all the code looks good.
CI log says:
|
The automated tests are actually more comprehensive than the manual tests I performed. Manually, I only created and updated a policy file to check the output, and then tried to update wiht the same content to watch it fail. I didn't actually create any volumes to use a policy manually. The only reason I did the manual tests was to sanity check my vsan/nimbus install and see the output before I ran |
I still see
in the CI log (which still shows as PASSED). Please take a look (the rest looks good, after merge conflicts cleanup) |
failed_updates.append(volume_name) | ||
|
||
if len(failed_updates) != 0: | ||
# All volumes failed to update, so reset the original policy |
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.
nit: All => Some
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 meant All. I just moved it inside the next conditional to be more clear.
Overall LGTM. Some minor suggestions above. |
@msterin I have no idea why failing tests wouldn't cause CI to fail. Clearly they are failing correctly due to lack of space and being marked as failures by python test infrastructure. |
Add unit test for vsan_policy.update in admin cli Add docs for vsan policy update in admin cli Tested via make all and CI, as well as manual tests. Fixes #329
6782102
to
edce62a
Compare
This branch was rebased on main and comments were addressed. |
LGTM - but please wait with merge until VSAN in CI is back on-line and CI passes (/cc @kerneltime ) |
LGTM |
Add unit test for vsan_policy.update in admin cli
Add docs for vsan policy update in admin cli
Tested via make all and CI, as well as manual tests.
Fixes #329