resolve ansible-test issues

[richm]

Lots of python code changes for formatting, fix linter issues,
etc. This also cleans up the module docs. One fix is that
for the RETURN parameters the returned: field is required.
I made all of the returned: success - I'm not sure if that
is really the case, or if some parameters are returned even if
the module failed, but I think success is safe.

[richm]

since there are a few PRs in flight, I think we should not merge this right away - it will be a big disruption - I'll have to rebase this and merge it after we get in ths PRs for 8.4

[nhosoi]

LGTM

Looking at this change, is this the first time to run black and flake8 on the storage role? (no wonder there were lots of formatting changes... :)

[richm]

LGTM

Looking at this change, is this the first time to run black and flake8 on the storage role? (no wonder there were lots of formatting changes... :)

Yes. Since we have to make other big changes for ansible-test, I figured we should enable black and flake8 too.

[nhosoi]

lgtm

(may not be merged just now, but you have my approval.)

[richm]

@nhosoi the 3 dependent PRs are merged - this PR is rebased on top of them - this PR also has the doc fixes

[richm]

rebase report - what affect with this have on existing PRs:

• Refined safe_mode condition in create_members #190 - minor change - richm@414db72
• Add new test case for bz1894647 #189 - rebase is clean
• Add lvm vdo test cases #187 - rebase is clean
• Fixed volume relabeling #185 - needs rebase to master first
• LVMVDO support #181 - needs rebase to master first
• fix: RAID volume pre cleanup  #169 - rebase is clean
• add new case for luks raid #150 - rebase is clean
• Add raid 1 5 6 10, Add one disk to tests/provision.fmf #149 - needs rebase to master first
• Coding style update #121 - needs rebase to master first
• Updated version of #50 #104 - rebase is clean
• Add 4 cases to cover the long name lvm creating rules #94 - rebase is clean
• Verify that disk volumes are mounted by UUID in fstab #50 - close in favor of Updated version of #50 #104?
• Assert that "get required packages" has not changed mounts #47 - rebase is clean
• percentage-based volume size (lvm only) #39 - minor rebase conflict

So wdyt? Can we merge this, then I help PR authors rebase their changes?

[richm]

ok to merge?

[dwlehman]

A lot of these changes seem like bikeshedding to me, but I don't feel strongly about it.

[dwlehman]

How this can be considered an improvement is beyond my comprehension, but I suppose I can read it either way.

[richm]

python black - this is a double edged sword - it fixes many legitimate issues found by Galaxy importer, but it imposes a rather arbitrary, and in some cases hard-to-read, formatting regime

[richm]

Every other system role uses python black formatting - if storage doesn't want to use this, we'll have to figure out some way to make an exception, and figure out how to lint some of the issues found by Galaxy importer without using black

[dwlehman]

No, it's probably worth it for the consistency and linting.

[richm]

ok - here's #205 for comparison - pretty much everything except the black formatting

[richm]

@dwlehman Can we get one of these PRs - this one or #205 - merged soon?

[richm]

[citest pending]

[richm]

[citest bad]

[richm]

[citest pending]

[nhosoi]

@richm, Is #205 a superset of this pr? Or independent?
Took a look quickly, some changes are in common, but others are not...
Probably, we need to merge both of them, but the second merge may give us some conflicts...

[richm]

@richm, Is #205 a superset of this pr? Or independent?

Independent. @dwlehman expressed concerns with the large number of formatting changes in this PR, so I created #205 as an alternative which fixed the ansible-lint issues without some of the large formatting changes, so we could compare the two and decide which one was better for the storage role.

Took a look quickly, some changes are in common, but others are not...
Probably, we need to merge both of them, but the second merge may give us some conflicts...

We need to choose one and close the other.

[dwlehman]

My comment was not intended to be a serious objection, just a comment that I personally find the new style unpleasant.

We should go with this version if it brings the project in line with the rest of the system roles in some meaningful way.

[richm]

closing in favor of #205