-
Notifications
You must be signed in to change notification settings - Fork 50
New Policy Regarding Python Compatibility for Collections #151
Conversation
Shouldn't we say something about which exceptions are ok (like not supporting Python 2.6 or Python 2 or Python 3.5, if explicitly documented)? And what we consider not to be a good idea (only supporting Python 3.8/3.9 for example)? |
@felixfontein Yeah that'll be a good idea but again I think it'll conflict with what we say in CI Testing section below. Any thoughts?
|
collection_checklist.md
Outdated
@@ -15,6 +15,7 @@ Every comment should say whether the reviewer expects it to be addressed, or whe | |||
- [ ] follows licensing rules | |||
- [ ] follows the [Ansible documentation standards](https://docs.ansible.com/ansible/devel/dev_guide/developing_modules_documenting.html) and the [style guide](https://docs.ansible.com/ansible/devel/dev_guide/style_guide/index.html#style-guide) | |||
- [ ] follows [development conventions](https://docs.ansible.com/ansible/devel/dev_guide/developing_modules_best_practices.html) | |||
- [ ] supports Python 2.6 or greater and Python 3.5 or greater |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should be more specific here.
python2.6 / python2.7 / python3.5 are all EOL. Expecting a new collection to support them, doesn't make too much sense. I think it is fair to say these are optional, if you want to support them.
https://www.python.org/downloads/ could be a good format
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See ansible/ansible#72668 (comment) which shows that some OS still support Python 2.7 till 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think 2.6+ and 3.5+ is the right set.
I think the mention in README.md should mainly be required if most/all modules/plugins have this requirement, not if some random modules/plugins have different requirements. Alternatively, we should accept a blanket statement like |
+1 for the alternative:) |
@Ompragash we were discussing about Python versions during today's community meeting. We agreed by vote on the following:
Can you integrate / add this to your proposal? Also, before we switched topic, I proposed for the remaining versions (2.7, 3.6, 3.7, 3.8, ...): "we could require that these are supported (and 3.8 and 3.9), with the possible exception that a library used does not support them". We wanted to continue discussing this the next meeting. Do you have suggestions, or other / alternative suggestions? It would probably be great if we could have a version of this PR that reflects something like all the above, so that we can vote on the PR (and merge if it is approved). Or at least have a new version so it can be approved the week thereafter. |
I couldn't attend the meeting, but something both @gundalow and @abadger referenced hits at the heart of it.
In the case of network, a lot of the content is run on the controller side. Given that ansible-core has stricter python requirements moving forward, we'd like the ability for that too. ansible-core 2.11 / 2.12 would never support python2.7 / python3.6 / python3.7 on the controller side, having the collection support it seems inconsistent. That really the gist of our point. |
@pabelanger while ansible-core 2.11 is planning to announce that it requires Python 3.8+, it still works fine with Python 2.7 and Python 3.5+. So the restricting to Python 3.8+ on the controller side effectively won't happen before ansible-core 2.12, and thus not before Ansible 5.0.0. |
That is true, but there is a rather large warning in 2.11 that if you are running less then python3.8 you get a big deprecation warning: Which I read it, it works but should really be using python3.8. I think it is a fair ask for a collection to decide if they want to even support deprecated python versions or make the jump to the latest sooner. |
@felixfontein adding the details around Python and OS supportability seems reasonable and I'll add the above changes to this PR that reflects like below:
|
@Ompragash looks good. Can you uppercase the |
@felixfontein consider it done :) |
Co-authored-by: Felix Fontein <felix@fontein.de>
Co-authored-by: Andrew Klychkov <aaklychkov@mail.ru>
Co-authored-by: Andrew Klychkov <aaklychkov@mail.ru>
Co-authored-by: Andrew Klychkov <aaklychkov@mail.ru>
Co-authored-by: Felix Fontein <felix@fontein.de>
Hi @Ompragash ! Despite being superficially simple, this is turning out to be a controversial policy :-) We have about five points of contention to resolve regarding this and at yesterday's meeting we made decisions on two of them which are related: One point we think is important is to have different Python requirements for the controller-side and remote-side. On the controller-side, the python versions required may be higher than what is required on the remote-side. So we would like two sets of python requirements rather than the current draft's single set. (Deciding what these versions are is one of the remaining points of contention). Figuring that out then lead us to the need to define what
One example where the even if clause comes into play is with cloud modules. These are commonly run on the controller but in some environments, the controller might run on one system inside a corporate network which can't directly access the cloud machines. The user has to have the modules run on a bastion host which has access to the cloud. We also note that the requirements for module_utils needs to be hashed out. Networking says they have some things which are modules (and so they can only pull their requirements from module_utils) but it's only possible to use the modules controller-side. Since we can't control what third parties do with those module_utils, we may want to make them obey the remote-side requirements. OTOH, that means the module_utils will have to support a larger set of python versions than the networking modules which form the bulk of their usage. The below proposals are the verbatim decisions we approved yesterday:
|
At today's community meeting, we had some more decisions for Python version requirements:
|
@abadger @felixfontein I modified the proposal based on the discussions/suggestions and also do you think we can change the term
|
"target" host/node is also an option |
@Ompragash Thanks for pointing that out. We talked about it in the meeting today and decided that "remote" and "side" were both confusing. So we came up with some different terms which are less linked to the ansible
|
And yes, I think a separate section for module_utils is fine.... they're the part with the most special cases so it makes sense that they have their own subsection |
Suggested changes applied and below is the updated proposal:
|
We decided during today's meeting that the descriptions in "In the controller environment, collections MUST support Python 2 (version 2.7) and Python 3 (Version 3.6 and higher), unless required libraries do not support these versions. Collections SHOULD also support Python v3.5 if all required libraries support this version." "In the other environment, collections MUST support Python 2 (version 2.7) and Python 3 (Version 3.6 and higher), unless required libraries do not support these versions. Collections SHOULD also support Python v2.6 and v3.5 if all required libraries support this version. It is important that we don't use controller node / managed node here, as this is not about nodes, but about environments. |
@felixfontein @abadger @gundalow updated the proposal and committed the changes as well! Kindly review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nits
Co-authored-by: David Moreau Simard <moi@dmsimard.com> Co-authored-by: Sandra McCann <samccann@redhat.com> Co-authored-by: Toshio Kuratomi <a.badger@gmail.com>
Co-authored-by: Toshio Kuratomi <a.badger@gmail.com>
As discussed and voted on in today's community meeting, we merge this with the above suggestions added. |
@Ompragash thanks a lot for working on this for so long! I'm really happy we finally got this endless topic merged ;) |
SUMMARY
ISSUE TYPE