Fixed the default behaviour of the autopartitioning (#1380767) #941
Conversation
poncovka
force-pushed
the
rhel7-branch-autopart_type
branch
from
e07325b
to
4576899
Compare
|
Great +1 for this! |
| @@ -290,7 +290,7 @@ def execute(self, storage, ksdata, instClass): | |||
|
|
|||
| # sets up default autopartitioning. use clearpart separately | |||
| # if you want it | |||
| instClass.setDefaultPartitioning(storage) | |||
| refreshAutoSwapSize(storage) | |||
There was a problem hiding this comment.
Please also describe this particular change in the commit message.
pyanaconda/installclass.py
Outdated
| @@ -58,6 +58,10 @@ class BaseInstallClass(object): | |||
| # Blivet uses by default. | |||
| defaultFS = None | |||
|
|
|||
| # The default partitioning scheme to use for autopartitioning. | |||
| # If None, we will use DEFAULT_AUTOPART_TYPE. | |||
There was a problem hiding this comment.
Why is it being set to None then?
There was a problem hiding this comment.
I could change None to DEFAULT_AUTOPART_TYPE, but then it is not necessary to have the constant DEFAULT_AUTOPART_TYPE at all, because it will be used only here.
There was a problem hiding this comment.
Still better to have a constant for this -- for other installclasses, addons,...
pyanaconda/iutil.py
Outdated
| """Return the first argument that is not None.""" | ||
| for arg in args: | ||
| if arg is not None: | ||
| return arg |
There was a problem hiding this comment.
This whole function can be written as:
return next((arg for arg in args if arg is not None), None)
There was a problem hiding this comment.
And if it is a function, it should be generic -- i.e. take any iterable as a single argument and return the first item that is not None. AFAICT there's no need to use *args machinery here.
There was a problem hiding this comment.
I've seen this implementation before, but it is really hard to read. What are the advantages?
There was a problem hiding this comment.
I can make the function generic, though.
There was a problem hiding this comment.
There's no advantage, it just looks more "pythonic", whatever that means. :) Feel free to leave the for cycle in place. My main concern here is the (non-)genericity.
| from pykickstart.errors import KickstartValueError | ||
|
|
||
| from collections import OrderedDict | ||
|
|
||
| import logging | ||
| log = logging.getLogger("anaconda") | ||
|
|
||
| __all__ = ["StorageSpoke", "AutoPartSpoke"] | ||
| __all__ = ["StorageSpoke"] |
There was a problem hiding this comment.
Is this change intentional? What is the purpose?
There was a problem hiding this comment.
In tui, it does not make sense to enable automatic import of indirect spokes, because we always create them and use them only in other spokes. With automatic import, they will be found by a hub, created, initialized and forgotten.
Also, I've added a new parameter to the AutoPartSpoke.__init__ method and the initialization in a hub does not work anymore.
There was a problem hiding this comment.
Thanks for the extra explanation. I think this deserves a separate commit or at least some explanation in the commit message, though.
poncovka
force-pushed
the
rhel7-branch-autopart_type
branch
from
4576899
to
f56e920
Compare
The default partitioning of the installclass should be always set. Then it is not necessary to set it again, but always refresh the size of swap before autopartitioning. Resolves: rhbz#1380767
The default partitioning scheme can be defined in an install class. If the kickstart file defines the scheme, it will be used instead. Resolves: rhbz#1380767
In tui, it does not make sense to enable automatic import of indirect spokes, because we always create them and use them only in other spokes. With automatic import, they will be found by a hub, created, initialized and forgotten.
poncovka
force-pushed
the
rhel7-branch-autopart_type
branch
from
f56e920
to
722dd3e
Compare
Always set the default partitioning
Enable to define the autopart type in an install class
If the kickstart file defines the scheme, it will be used instead.
Resolves: rhbz#1380767