[WIP] fleetctl destroy fix and more tests #1435
Conversation
* tryWaitForUnitStates() tries to wait for units to reach the desired state. * getBlockAttempts() gets the correct value of how many attempts to try before giving up on an operation. These helpers will be used to make the code more consistent and clean. We do not intended to change any behaviour here. ---
…disk or the Registry
* Cover code path of getUnit*() functions with debug messages * Improve code comments
Just make it a const var that can be used or adapted if needed later.
Do not error out directly if Destroy command fails, check first if the unit does really exist if no then ignore the destroy error and continue. Follow-up fix for: coreos#1383
…e conditions Make the test more smarter by checking for race conditions and output result. Sometimes you may see normal output sometimes no output between the two goroutines which is normal, the thing to worry about is if Destroy did success or not.
Fix and enforce the check for the intended desired state.
More tests for the Inactive desired state.
|
General comment: it would be immensely easier to review and land these more quickly if the PRs were in bite sized chunks. It's unclear to me that this needs to be based on #1433 |
|
e.g. endocode@fd173a0 could be in its own PR, as could say |
|
Alright! noted! For your comment: "hmm? now we're ignoring errors.." these are just unit tests some logic could be used later for functional ones, but I wanted to standardize fleectl commands first, or am I missing something ? Thanks! |
|
@jonboulle got you! So the new PR is here: #1439 so yes I added a new function IsErrorUnitNotFound() part of the client package but not an exported cAPI, if later we want to check for more error codes in that path we may consider something IsErrorCode(err error, code int)... and export it. For the other patches that can be put on there own PR, yes I do agree, but at the same time these changes were done in the same process of improving this code and adding more tests, some of these functions are new and are only used by these new tests. Thanks! |
|
Closed in favor of #1439 |
Fix for fleetctl destroy command and more tests.
@jonboulle when you take a look please start from the patch: "fleetctl: avoid hard coding sleep time values inside calls" since this branch is on top of #1433