Potential deprecation in RHEL 7.3 ADDON API

[Jiří Konečný]

I have RFE which required to add payload class to addons so I created
this PR:
https://github.com/rhinstaller/anaconda/pull/571

This PR will cause that all existing addons to get deprecation warning
message. This includes our OSCAP and KDUMP addons.

The change is really small. You only need to change two lines of code.
I'm going to create PR for our addons when this PR receive ACK.

This change happens on master and it will be also in RHEL 7.3 .
I want to give you notification about this change before it happens by
this mail. If I missed something or you don't want this change please
write comments to PR or reply to this mail.

Thank you,
Jirka K.

[Chris Lumens]

> This PR will cause that all existing addons to get deprecation warning
> message. This includes our OSCAP and KDUMP addons.
>
> The change is really small. You only need to change two lines of code.
> I'm going to create PR for our addons when this PR receive ACK.
>
> This change happens on master and it will be also in RHEL 7.3 .
> I want to give you notification about this change before it happens by
> this mail. If I missed something or you don't want this change please
> write comments to PR or reply to this mail.

You can avoid having to deal with deprecation warnings and all that if
you instead define execute to also take *args and **kwargs, and then
pass payload to it as a kwarg everywhere.  Addons that don't do that
just won't have access to the payload, but that's their problem.

- Chris

[Martin Kolman]

----- Original Message -----
> From: "Chris Lumens" 
> To: [e-mail]
> Sent: Wednesday, March 30, 2016 2:36:12 PM
> Subject: Re: Potential deprecation in RHEL 7.3 ADDON API
> > This PR will cause that all existing addons to get deprecation warning
> > message. This includes our OSCAP and KDUMP addons.
> >
> > The change is really small. You only need to change two lines of code.
> > I'm going to create PR for our addons when this PR receive ACK.
> >
> > This change happens on master and it will be also in RHEL 7.3 .
> > I want to give you notification about this change before it happens by
> > this mail. If I missed something or you don't want this change please
> > write comments to PR or reply to this mail.
>
> You can avoid having to deal with deprecation warnings and all that if
> you instead define execute to also take *args and **kwargs, and then
> pass payload to it as a kwarg everywhere.  Addons that don't do that
> just won't have access to the payload, but that's their problem.

I think that the problem is that they might have overridden the method in their addon,
so that when we call it with an unknown kwarg they will get an exception.

They might have something like this in their addon code:

def execute(self, storage, ksdata, instclass, users):

Even if we change the parent class method signature to:

def execute(*args, **kwargs):

The overridden method will still be called when we call execute with payload=<payload instance>
and they will get:

TypeError: execute() got an unexpected keyword argument 'payload'

I can see two more or less hacky ways of working around this without breaking backward compatibility for
addons (which I assume should be maintained on RHEL, mainly due to custom addons customers might have):

1) Catch the type error and retry without the payload kwarg:

try:
    ad.execute(foo, bar, baz, payload="ABC")
except TypeError:
    ad.execute(foo, bar, baz)

2) Use the inspect module to check if the method expects the payload kwarg (or **kwargs)
and only call it with it if it does. Note that this is ultra-bleh-hacky and probably
also pretty un-Pythonic.
> Chris
>
> Anaconda-devel-list mailing list
> [e-mail]
> https://www.redhat.com/mailman/listinfo/anaconda-devel-list

[Jiří Konečný]

On Wed, 2016-03-30 at 09:02 -0400, Martin Kolman wrote:
> ----- Original Message -----
> > From: "Chris Lumens" 
> > To: [e-mail]
> > Sent: Wednesday, March 30, 2016 2:36:12 PM
> > Subject: Re: Potential deprecation in RHEL 7.3 ADDON API
> > > This PR will cause that all existing addons to get deprecation
> > > warning
> > > message. This includes our OSCAP and KDUMP addons.
> > >
> > > The change is really small. You only need to change two lines of
> > > code.
> > > I'm going to create PR for our addons when this PR receive ACK.
> > >
> > > This change happens on master and it will be also in RHEL 7.3 .
> > > I want to give you notification about this change before it
> > > happens by
> > > this mail. If I missed something or you don't want this change
> > > please
> > > write comments to PR or reply to this mail.
> >
> > You can avoid having to deal with deprecation warnings and all that
> > if
> > you instead define execute to also take *args and **kwargs, and
> > then
> > pass payload to it as a kwarg everywhere.  Addons that don't do
> > that
> > just won't have access to the payload, but that's their problem.
>
> I think that the problem is that they might have overridden the
> method in their addon,
> so that when we call it with an unknown kwarg they will get an
> exception.
>
> They might have something like this in their addon code:
>
> def execute(self, storage, ksdata, instclass, users):
>
> Even if we change the parent class method signature to:
>
> def execute(*args, **kwargs):
>
> The overridden method will still be called when we call execute with
> payload=<payload instance>
> and they will get:
>
> TypeError: execute() got an unexpected keyword argument 'payload'
>
> I can see two more or less hacky ways of working around this without
> breaking backward compatibility for
> addons (which I assume should be maintained on RHEL, mainly due to
> custom addons customers might have):
> Catch the type error and retry without the payload kwarg:
>
> try:
>     ad.execute(foo, bar, baz, payload="ABC")
> except TypeError:
>     ad.execute(foo, bar, baz)

I tried this and trash it later.
Problem here is that you also catch every TypeError in the code of the
addon. For example calling list as a method.
Workaround for this could be to test what is written in exception but
it is uglier and less stable than actual solution.
> Use the inspect module to check if the method expects the payload
>
> kwarg (or **kwargs)
> and only call it with it if it does. Note that this is ultra-bleh-
> hacky and probably
> also pretty un-Pythonic.

I think so too and from point of view I have now. It's better to have
actual solution.

Jirka
> > Chris
> >
> > Anaconda-devel-list mailing list
> > [e-mail]
> > https://www.redhat.com/mailman/listinfo/anaconda-devel-list
>
> Anaconda-devel-list mailing list
> [e-mail]
> https://www.redhat.com/mailman/listinfo/anaconda-devel-list

[Alexander Todorov]

На 30.03.2016 в 16:02, Martin Kolman написа:
> Use the inspect module to check if the method expects the payload kwarg (or **kwargs)
>
> and only call it with it if it does. Note that this is ultra-bleh-hacky and probably
> also pretty un-Pythonic.

I've tried this approach before. Info and examples here:

http://atodorov.org/blog/2015/11/29/inspecting-method-arguments-in-python/

I will work but indeed looks quite hacky.

--
Alex

[Jiří Konečný]

On Thu, 2016-03-31 at 09:39 +0300, Alexander Todorov wrote:
> На 30.03.2016 в 16:02, Martin Kolman написа:
> > Use the inspect module to check if the method expects the
> >
> > payload kwarg (or **kwargs)
> > and only call it with it if it does. Note that this is ultra-bleh-
> > hacky and probably
> > also pretty un-Pythonic.
>
> I've tried this approach before. Info and examples here:
>
> http://atodorov.org/blog/2015/11/29/inspecting-method-arguments-in-py
> thon/
>
> I will work but indeed looks quite hacky.

I'm using now similar approach than you. But without the **kwargs and
*args solution and without the need to import inspect. However from my
understanding inspect module is doing the same as me.

I'm only using attributes from this table 
https://docs.python.org/2/library/inspect.html%C2%A0.

Jirka
> --
> Alex
>
> Anaconda-devel-list mailing list
> [e-mail]
> https://www.redhat.com/mailman/listinfo/anaconda-devel-list

[Jiří Konečný]

On Wed, 2016-03-30 at 08:36 -0400, Chris Lumens wrote:
> > This PR will cause that all existing addons to get deprecation
> > warning
> > message. This includes our OSCAP and KDUMP addons.
> >
> > The change is really small. You only need to change two lines of
> > code.
> > I'm going to create PR for our addons when this PR receive ACK.
> >
> > This change happens on master and it will be also in RHEL 7.3 .
> > I want to give you notification about this change before it happens
> > by
> > this mail. If I missed something or you don't want this change
> > please
> > write comments to PR or reply to this mail.
>
> You can avoid having to deal with deprecation warnings and all that
> if
> you instead define execute to also take *args and **kwargs, and then
> pass payload to it as a kwarg everywhere.  Addons that don't do that
> just won't have access to the payload, but that's their problem.
> Chris

I think what wrote mkolman is accurate. As a bonus when we want to do
this we want to do this everywhere for addons. If am I not mistaken it
will be on 6 places only on the addon part (GUI, TUI, KS sections) and
every call on these methods in the Anaconda.

If we will go for for **kwargs/*args solution then I think we should
create new methods execute and setup to distinguish from the old code.

Jirka
> Anaconda-devel-list mailing list
> [e-mail]
> https://www.redhat.com/mailman/listinfo/anaconda-devel-list

[Jiří Konečný]

If everyone is fine with my solution I'm going to push it next week.
And I'll start to fix the addons before it.

Jirka

On Wed, 2016-03-30 at 13:00 +0200, Jiří Konečný wrote:
> I have RFE which required to add payload class to addons so I created
> this PR:
> https://github.com/rhinstaller/anaconda/pull/571
>
> This PR will cause that all existing addons to get deprecation
> warning
> message. This includes our OSCAP and KDUMP addons.
>
> The change is really small. You only need to change two lines of
> code.
> I'm going to create PR for our addons when this PR receive ACK.
>
> This change happens on master and it will be also in RHEL 7.3 .
> I want to give you notification about this change before it happens
> by
> this mail. If I missed something or you don't want this change please
> write comments to PR or reply to this mail.
>
> Thank you,
> Jirka K.
>
> Anaconda-devel-list mailing list
> [e-mail]
> https://www.redhat.com/mailman/listinfo/anaconda-devel-list

[Pat Riehecky]

Not that my vote really counts, but it looks good to me!

Pat

On 04/15/2016 10:38 AM, Jiří Konečný wrote:
> If everyone is fine with my solution I'm going to push it next week.
> And I'll start to fix the addons before it.
>
> Jirka
>
> On Wed, 2016-03-30 at 13:00 +0200, Jiří Konečný wrote:
> > I have RFE which required to add payload class to addons so I created
> > this PR:
> > https://github.com/rhinstaller/anaconda/pull/571
> >
> > This PR will cause that all existing addons to get deprecation
> > warning
> > message. This includes our OSCAP and KDUMP addons.
> >
> > The change is really small. You only need to change two lines of
> > code.
> > I'm going to create PR for our addons when this PR receive ACK.
> >
> > This change happens on master and it will be also in RHEL 7.3 .
> > I want to give you notification about this change before it happens
> > by
> > this mail. If I missed something or you don't want this change please
> > write comments to PR or reply to this mail.
> >
> > Thank you,
> > Jirka K.
> >
> > Anaconda-devel-list mailing list
> > [e-mail]
> > https://www.redhat.com/mailman/listinfo/anaconda-devel-list
>
> Anaconda-devel-list mailing list
> [e-mail]
> https://www.redhat.com/mailman/listinfo/anaconda-devel-list

[Jiří Konečný]

On Fri, 2016-04-15 at 10:45 -0500, Pat Riehecky wrote:
> Not that my vote really counts, but it looks good to me!
>
> Pat

Hi Pat,

we are Open Source, everyone has their vote ;).