Configuration actions

[vojtechtrefny]

This is not a final version, just a "request for comments".

I've decided to use the "single class" approach with a dict of supported ("configurable") attributes and functions used to set the attribute. I didn't have to create a special function for label configuration -- we already have the write_label functions so I've just changed it to take the dry_run option for the input validation.

Pros:

• It's easy to tell if format supports configuration of an attribute using actions.
• Having only one class makes the API simpler for the users.
• It's possible to use existing validation functions/tasks.

Cons:

• Every attribute needs a special function that looks like a setter but it actually isn't (because of the dry_run option).

[vpodzime]

Looks good to me.

[dwlehman]

I notice this doesn't really do any explicit checking of the new value. I think any errors would come up in apply, which I'm not sure is what we want -- I think it might be better to check it from the constructor.

Also, I'm not sure what the value is of setting self._run in the action class.

Otherwise I think it looks good.

[vojtechtrefny]

I notice this doesn't really do any explicit checking of the new value. I think any errors would come up in apply, which I'm not sure is what we want -- I think it might be better to check it from the constructor.

I wans't sure if the checks should go to apply or to constructor, I'll move it.

Also, I'm not sure what the value is of setting self._run in the action class.

I just didn't want to have the same code (getattr) twice in apply and execute. Maybe the name is a little misleading.

[dwlehman]

Yes, I think if you named it self._execute or self._commit or something else fairly explicit it would be okay. I think you should also do something along the lines of assert(callable(self._execute)) right after setting it.

[dwlehman]

I also think we can do better than conf_actions. I'm not sure what would be best, but config_actions or maybe even config_actions_map or config_action_methods come to mind.

[vojtechtrefny]

New version -- added ActionCofigureDevice, checks moved to action constructor, changed same attribute/function names and added some basic test for configure action functionality.

[vpodzime]

What is the purpose and result of this action?

[vojtechtrefny]

This is intended to change (future) mountpoint for existing devices during installation or some "installation mode". It indeed is quite a weird action because it does nothing during execute, it just set the attribute.

[vpodzime]

Looks good to me other than the above question.

[dwlehman]

if self._execute is not None: would be better.

[vojtechtrefny]

Fixed.

[dwlehman]

Overall I like it, but I have some concern about the device and format configuration action classes being almost identical. I realize that trying to combine them into a single class brings its own issues (and complexity), but it seems like asking for trouble requiring every change to made in two classes rather than one.

[vojtechtrefny]

I think it makes sense to have two actions because we already have two actions for everything -- API users probably might expect to have a different actions for devices and formats.

Maybe an abstract class with common functionality could be useful.

[vojtechtrefny]

Removed the mountpoint configuration commit.

[vpodzime]

Looks good to me.

[vojtechtrefny]

One PEP8 fix for a different change was part of the removed commit, fixed.