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]

Post by Jiří Konečný
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 -----

Sent: Wednesday, March 30, 2016 2:36:12 PM
Subject: Re: Potential deprecation in RHEL 7.3 ADDON API

Post by Jiří Konečný
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
https://www.redhat.com/mailman/listinfo/anaconda-devel-list

[Jiří Konečný]

Post by Martin Kolman
----- Original Message -----

Sent: Wednesday, March 30, 2016 2:36:12 PM
Subject: Re: Potential deprecation in RHEL 7.3 ADDON API

Post by Jiří Konečný
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.
The overridden method will still be called when we call execute with
payload=<payload instance>
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
    ad.execute(foo, bar, baz, payload="ABC")
    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.

Post by Martin Kolman
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.

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

Jirka

Post by Martin Kolman

- Chris
_______________________________________________
Anaconda-devel-list mailing list
https://www.redhat.com/mailman/listinfo/anaconda-devel-list

_______________________________________________
Anaconda-devel-list mailing list
https://www.redhat.com/mailman/listinfo/anaconda-devel-list

[Alexander Todorov]

Post by Martin Kolman
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.

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ý]

Post by Alexander Todorov

Post by Martin Kolman
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.

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 .

Jirka

Post by Alexander Todorov
--
Alex
_______________________________________________
Anaconda-devel-list mailing list
https://www.redhat.com/mailman/listinfo/anaconda-devel-list

[Jiří Konečný]

Post by Chris Lumens

Post by Jiří Konečný
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

Post by Chris Lumens
_______________________________________________
Anaconda-devel-list mailing list
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

Post by Jiří Konečný
I have RFE which required to add payload class to addons so I created
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
https://www.redhat.com/mailman/listinfo/anaconda-devel-list

[Pat Riehecky]

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

Pat

Post by 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

Post by Jiří Konečný
I have RFE which required to add payload class to addons so I created
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
https://www.redhat.com/mailman/listinfo/anaconda-devel-list

_______________________________________________
Anaconda-devel-list mailing list
https://www.redhat.com/mailman/listinfo/anaconda-devel-list

[Jiří Konečný]

Post by Pat Riehecky
Not that my vote really counts, but it looks good to me!
Pat

Hi Pat,

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