Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add Atomic install class #1311

Merged
merged 4 commits into from Feb 9, 2018
Merged

Add Atomic install class #1311

merged 4 commits into from Feb 9, 2018

Conversation

jkonecny12
Copy link
Member

Migrate Atomic install class to Anaconda.
Add productVariant check and fix stylesheet in the atomic install class.

The Atomic iso (Fedora-Atomic-ostree-x86_64-Rawhide-20180117.n.1.iso) already have productVariant in the .buildstamp file and stylesheet in the fedora logos package so this install class should work without problems -- tested with updates image. The only thing missing is to deprecate the fedora-productimg-atomic package.

Resolves: rhbz#1536853
Resolves: rhbz#1491287

@jkonecny12 jkonecny12 added the master Please, use the `f39` label instead. label Jan 23, 2018
@jkonecny12 jkonecny12 force-pushed the master-add-atomic-install-class branch from 5a5e6f3 to 9cda077 Compare January 23, 2018 12:55
poncovka
poncovka approved these changes Jan 23, 2018
View reviewed changes
Copy link
Contributor

@poncovka poncovka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

cgwalters
cgwalters reviewed Jan 23, 2018
View reviewed changes
Copy link
Contributor

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just an optional suggestion, otherwise looks sane to me.

pyanaconda/installclasses/installclass_atomic.py Outdated
FedoraBaseInstallClass.__init__(self)

# This is intended right now to match Fedora Server; if changing this,
# please discuss on https://lists.projectatomic.io/projectatomic-archives/atomic-devel/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side note...now that this is in the same codebase as Server, maybe we could actually dedup this? Not a blocker/requirement for merging though from my PoV.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've used default partitioning from Fedora Server in the Fedora Atomic install class. Is that what you meant @cgwalters ?

@jkonecny12 jkonecny12 force-pushed the master-add-atomic-install-class branch 2 times, most recently from 5f912c0 to 85d481b Compare January 24, 2018 12:56
@jkonecny12
Copy link
Member Author

UDPATED:

  • use __all__ in Atomic install class
  • use default partitioning in Atomic from Server variant

@AdamWill
Copy link
Contributor

AdamWill commented Feb 1, 2018

ping on this? Atomic installer images for Rawhide are still broken.

poncovka
poncovka suggested changes Feb 5, 2018
View reviewed changes
Copy link
Contributor

@poncovka poncovka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some suggestions.

pyanaconda/installclasses/fedora_server.py Outdated
@@ -37,6 +37,10 @@ class FedoraServerInstallClass(FedoraBaseInstallClass):
hidden = True

def setDefaultPartitioning(self, storage):
FedoraServerInstallClass.createDefaultPartitioning(storage)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that you can use self here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

pyanaconda/installclasses/installclass_atomic.py
FedoraBaseInstallClass.__init__(self)

def setDefaultPartitioning(self, storage):
FedoraServerInstallClass.createDefaultPartitioning(storage)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this install class be based on Fedora Server install class?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again I'm not sure we want to do that. Yeah this one method is the same but what if FedoraServer will change something else in future.

Basically I don't think we want to mix hierarchy in this. If something we can use that mixin but these install classes are not extending one to other by principle.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I don't like this solution, but we will change install classes anyway.

pyanaconda/installclasses/installclass_atomic.py Outdated
class AtomicInstallClass(FedoraBaseInstallClass):
name = "Atomic Host"
stylesheet = "/usr/share/anaconda/pixmaps/atomic/fedora-atomic.css"
sortPriority = 11000
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Priority should be set to FedoraBaseInstallClass.sortPriority + 1.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

pyanaconda/installclasses/installclass_atomic.py Outdated
name = "Atomic Host"
stylesheet = "/usr/share/anaconda/pixmaps/atomic/fedora-atomic.css"
sortPriority = 11000
hidden = False
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Atomic install class can be used in Fedora and RHEL? Otherwise, this line should be removed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

pyanaconda/installclasses/fedora_server.py Outdated
FedoraServerInstallClass.createDefaultPartitioning(storage)

@staticmethod
def createDefaultPartitioning(storage):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this method could be provided by BaseInstallClass.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it is good idea. This is not the default partitioning. Or anything like that.
If we really want to do something like that then we should create new mixin class for it but I don't know where to put it how to name it and so on and I'm not sure if that would be that beneficial.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.

@jkonecny12
Move Atomic install class to Anaconda (#1491287) (#1536853)
0555e25 
Fix the incorrect import.

Resolves: rhbz#1536853
Resolves: rhbz#1491287
@jkonecny12
Migrate Atomic install class (#1491287)
048c880 
Fix stylesheet:
Stylesheet is used from fedora-logos package where the atomic css file
is placed.

Add product variant:
Product variant is a new way how to recognize Fedora variants to
specify special behavior and correctly choose install class. Product
variant is specified by Lorax and saved to the .buildstamp file[1].
Hide atomic install class based on this value.

Use sort priority from base class:
Base class have default sort priority so change the original constant
as base value plus one.

[1]: https://pagure.io/releng/issue/7202

Related: rhbz#1491287
@jkonecny12
Clean code of Atomic install class
d025d2f 
Remove unused imports.
Fix code style.
@jkonecny12
Use Fedora Server default partitioning in Atomic
2a6fd3a 
The partitioning should be the same for both variants.
@jkonecny12 jkonecny12 force-pushed the master-add-atomic-install-class branch from 85d481b to 2a6fd3a Compare February 8, 2018 11:29
@jkonecny12
Copy link
Member Author

UPDATED
Thanks for your suggestions, they should be implemented now.

@jkonecny12
Copy link
Member Author

jkonecny12 commented Feb 8, 2018 •
edited

@cgwalters Could you please verified the default partitioning from Fedora Server install class in Fedora Atomic?

I know there were different values but it seemed to me like they are not intentional.

poncovka
poncovka approved these changes Feb 8, 2018
View reviewed changes
Copy link
Contributor

@poncovka poncovka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Thanks!

@cgwalters
Copy link
Contributor

I know there were different values but it seemed to me like they are not intentional.

Right, I don't think there was a strong rationale for having them different. The new values look fine to me. (Big picture, for bare metal installations we generally expect sysadmins will want to control things. For cloud images we use separate kickstarts which is unrelated to this.)

@jkonecny12
Copy link
Member Author

jenkins, test this please

@jkonecny12 jkonecny12 merged commit 0369345 into rhinstaller:master Feb 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cgwalters cgwalters cgwalters left review comments

@poncovka poncovka poncovka approved these changes

Assignees
No one assigned
Labels
master Please, use the `f39` label instead.
Milestone
No milestone
4 participants
@jkonecny12 @AdamWill @cgwalters @poncovka