ec2_vol: Add check_mode support to ec2_vol

[mandar242]

SUMMARY

Add check_mode support to ec2_vol.

ISSUE TYPE

• Feature Pull Request

COMPONENT NAME

ec2_vol

[ansibullbot]

@mandar242 this PR contains the following merge commits:

• 86ebf55

Please rebase your branch to remove these commits.

click here for bot help

[ansibullbot]

cc @jillr @lwade @s-hertel @tremble @wimnat
click here for bot help

[goneri]

recheck

[alinabuzachis]

recheck

[alinabuzachis]

recheck

[alinabuzachis]

recheck

[jillr]

Thanks @mandar242. What you have here looks good but could you please add some tests for different update actions? As the code flows now it should be fine, but for example module_utils.ec2.ensure_ec2_tags doesn't handle check_mode so if the logic ever got changed around we could miss that if there wasn't a test for tag changes under check_mode.

[tremble]

module_utils.ec2.ensure_ec2_tags doesn't handle check_mode so if the logic ever got changed around we could miss that if there wasn't a test for tag changes under check_mode.

Not entirely true, it's handled down in remove_ec2_tags and add_ec2_tags which will return "changed" but not make the change in check mode.

[alinabuzachis]

@mandar242 Thank you very much for working on this. I have only one last suggestion. Could you please use as example the tests run without the check mode and add more tests for the check mode in an analogous way (where it makes sense)? We want to be sure the check mode covers all the functionalities.

[tremble]

To follow up on what @alinabuzachis said: where possible, the best way to write the tests is for each 'change' that you make within the tests you run the change 4 times:

1. With check_mode - testing is changed
2. Without check_mode - testing is changed and the other return values
3. With check_mode again, testing is not changed
4. Without check_mode again, testing is not changed and the other return values.

This can get rather long and repetitive for the more complex modules, but catches a lot of annoying little bugs and when someone's reviewing the code it's much easier to see that things like idempotency and check mode are behaving consistently within the module. I'll often label each test as

1. Update SomeThing - check_mode
2. Update SomeThing
3. Update SomeThing - idempotency - check_mode
4. Update SomeThing - idempotency

[tremble]

recheck

[jillr]

recheck

@tremble saw your ping in irc after you'd gone. That's a really interesting CI failure that I don't know about! Let's see if it happens again?

[jillr]

recheck

[alinabuzachis]

@mandar242 Thank you. LGTM!

[tremble]

One minor niggle for the future (not worth blocking this over). I find it's less likely to lead to weird edge cases if you make your check_mode tests as late as possible. Here for example, if someone re-wrote the flow of main() and you'd sometimes be passed a volume, performing the check_mode test before you test volume would result in broken idempotency.

[ansible-zuul]

LGTM!