Validated content feedback from @ericzolf

[swapdisk]

After a quick check:

1. not all points have been ticked, what is exactly the status?
2. I find the idea to name the collection like one of the roles questionable, perhaps a better name is possible, covering all aspects of the collection?
3. the variables haven't been named according to standard, with proper prefix
4. the tasks name are not prefixed with the name of the task file
5. the lvm_snapshots role is IMHO too complex and should be split into one role for each action to keep it/them nice and tidy (and avoid too many skipped tasks)

Originally posted by @ericzolf in ansible/validated-content-discussion#20 (comment)

[swapdisk]

I kind of like the idea of suggestion 5. Separate roles with distinct names would be better and would also resolve suggestion 2 without having to change the name of the collection which I would rather not do.

Suggestions 3 and 4 seem reasonable.

Both changing variable names as per suggestion 3 and splitting the lvm_snapshots role out to one role for each action would be a major changes requiring bumping to a "2.0.0" release.

@ygalblum, your thoughts?

[ygalblum]

I like the idea of splitting the lvm_snaphosts role. Furthermore, I found that it contains the actions check_for_resize and resize which should be removed. resize is not even implemented in this role and they are both actually implemented in the shrink_lv role.

Item 3 seems required (though not sure why the ansible-lint did not err on it). Not sure is item 4 is part of the standard and to me looks redundant, but I don't mind complying.

We need to discuss if the collections' name is still correct, though I understand your concern about changing it and yes, it would require a 2.X release as it changes the API (role names)

[swapdisk]

With #40 merged, I think we have addressed all of the concerns. We'll wait for the validated content folks to take a fresh look before closing this.

[ericzolf]

I like the idea of splitting the lvm_snaphosts role. Furthermore, I found that it contains the actions check_for_resize and resize which should be removed. resize is not even implemented in this role and they are both actually implemented in the shrink_lv role.

We recommend to name roles like <object>_<action> because it alphabetically sorts more nicely, hence snapshot_{create,remove,revert}, but no big things, it won't keep me from approving (but you are still welcome to make me 100% happy 😉 ).

Item 3 seems required (though not sure why the ansible-lint did not err on it).

It's not linted because you can use a simpler/shorter prefix, which becomes difficult to validate automatically.

Not sure is item 4 is part of the standard and to me looks redundant, but I don't mind complying.

Why it is not redundant is explained here (tl;dr: it does make logs more readable and useful).
Prefix task names in sub-tasks files is only opt-in (I personally think that it helps a great deal to debug big roles with a lot of sub-tasks files).

We need to discuss if the collections' name is still correct, though I understand your concern about changing it and yes, it would require a 2.X release as it changes the API (role names)

I haven't seen the discussion, any decision? As the role has disappeared, it's probably fine anyway.

[swapdisk]

We recommend to name roles like <object>_<action> because it alphabetically sorts more nicely, hence snapshot_{create,remove,revert}, but no big things, it won't keep me from approving (but you are still welcome to make me 100% happy 😉 ).

I knew there was a reason not to push out the 2.0 release yet. I like this suggestion, so no problem renaming the roles to make you 100% happy!

[swapdisk]

@ericzolf, the snapshot roles are now renamed like <object>_<action> with just merged PR #50.