[PATCH 15/16] Add support for KS %addon section and the API+code to use it

Vratislav Podzimek vpodzime at redhat.com
Thu Dec 13 17:10:50 UTC 2012


On Thu, 2012-12-13 at 11:59 -0500, Martin Sivak wrote:
> > > +    def handle_line(self, line):
> > > +        """Process one kickstart line."""
> > > +        self.content += line
> > Does line end with the newline character?
> 
> Hmm.. good question, I have no idea :)
>  
> 
> > > +        if self.addon_id and not hasattr(self.handler.addon,
> > > self.addon_id):
> > > +            setattr(self.handler.addon, self.addon_id,
> > > AnacondaKSAddon(self.addon_id))
> > Why are you using setattr here?
> 
> Because self.addon_id (the id of the addon) is a string we got from the filesystem and I need to set an attribute with the same name?
Ah, of course, I somehow got confused. :)

> 
> > > +    with progress_report(_("Configuring addons")):
> >                               ^^^^ I guess we need a better word
> >                               here.
> 
> Probably, but we will have real 3rd party addons too here, not just externally maintained components
I don't like 'Configuring', 'addon' is another story.
> 
> > > topology = None
> > >  
> > > +
> > >  class AnacondaKSScript(KSScript):
> 
> > No newline needed here and the same goes for the AnacondaKSHandler
> > class.
> 
> That is actually PEP8 style.. two lines between classes. But I can remove it of course..
>
>  
> > > os.path.basename(os.path.dirname(os.path.abspath(path)))
> > Are all os.path.* calls needed here? Why to get abspath when you use
> > dirname and basename afterwards?
> 
> Because if any path contains "something/../spokes" you will get "something/.." out of dirname and ".." out of basename.
Ah, okay. Maybe there should be something like os.path.expandpath() for
these cases.

>  
> > > +            if not os.path.isdir(path):
> > > +                continue
> > > +
> > > +            classes = collect(module_name, path, lambda cls:
> > > issubclass(cls, self.AddonClassType))
> > > +            print classes
> > I guess we don't need this print.
> > 
> > > +            if classes:
> > > +                addons[addon_id] = classes[0](name = addon_id)
> > Could there be multiple classes? And if yes, how it is ensured that
> > the
> > right one is the first in the list?
> 
> Only one matching class per plugin was the plan. But this is still open to changes. I need to see real use cases from other teams first (ID handling would have to change if we allow more classes).
Okay.

-- 
Vratislav Podzimek

Anaconda Rider | Red Hat, Inc. | Brno - Czech Republic



More information about the anaconda-patches mailing list