ansible2: 2.2.0.0 -> 2.2.1.0 #22056

peterhoeg · 2017-01-23T13:18:55Z

Motivation for this change

2.2.1.0 (as well as 2.1.4) fix a fairly nasty CVE: https://lwn.net/Articles/711357/

We do a few more things at the same time:

a) ansible is an application, not a package, so move it out of python-packages.nix
b) version 2.1 is an upstream supported version, so add that too
c) create a generic builder for versions 1.9, 2.1 and 2.2
d) fix kargo that depends on python2Packages.ansible2 now that it has been moved out of pythonPackages.

This PR covers both security fixes, new branch of code (2.1.x) as well as refactoring of the builder (less code, yeay!!) which may be OK for unstable but not necessarily 16.09.

Should we mark ansible 1 as broken as no fix has been released?

@grahamc, how would like me to proceed?

Things done

Tested using sandboxing
(nix.useSandbox on NixOS,
or option build-use-sandbox in nix.conf
on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- Linux
Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Fits CONTRIBUTING.md.

mention-bot · 2017-01-23T13:19:05Z

@peterhoeg, thanks for your PR! By analyzing the history of the files in this pull request, we identified @FRidh and @jgeerds to be potential reviewers.

Mic92 · 2017-01-23T15:50:03Z

👍 for moving packages out of python-packages.nix. We actually do this also for python modules now (so only the reference stays in ./pkgs/top-level/python-packages.nix while the expressions move to python-modules)

Mic92 · 2017-01-23T15:52:40Z

pkgs/tools/admin/ansible/default.nix

+    sha256  = "0gz9i30pdmkchi936ijy873k8di6fmf3v5rv551hxyf0hjkjx8b3";
+  };
+
+  ansible  = ansible1;


When we do make to ansible2 the default ansible? Where did you find information regarding supported ansible versions?

Agreed, should we make ansible2 default?

If you do so please add it to the 17.03 release notes

I don't know of anything official from RedHat, but they specifically mentioned only releasing fixes for the 2.1 and 2.2 versions hence I'm assuming those are the only properly supported versions.

As for changing the default - that would only be in unstable, correct?

@globin, I'll switch the default in a separate PR and include docs.

jgeerds · 2017-01-23T16:01:17Z

@Mic92 I didn't know about this! Is there a official announcement / changelog ? I like that idea because python-packages.nix was/is bloated...

grahamc · 2017-01-23T16:06:03Z

(replying is on my to-do list for this afternoon)

grahamc · 2017-01-24T00:52:33Z

Yikes! Thank you!

Due to:

a) ansible is an application, not a package, so move it out of python-packages.nix

I think we should separately patch stable and merge this PR (which I think is a nice improvement.)

This PR covers both security fixes, new branch of code (2.1.x) as well as refactoring of the builder (less code, yeay!!) which may be OK for unstable but not necessarily 16.09.

Agreed, looks great :) thank you.

Should we mark ansible 1 as broken as no fix has been released?

I don't think so, not yet, let's hope they release an update soon?

Can you provide a patch for 16.09?

FRidh · 2017-01-24T06:27:41Z

a) ansible is an application, not a package, so move it out of python-packages.nix
d) fix kargo that depends on python2Packages.ansible2 now that it has been moved out of pythonPackages.

How does kargo use ansible? Does it import from it, or does it run it?

peterhoeg · 2017-01-24T09:11:57Z

@grahamc, ~~shouldn't we just patch the version for 2.2 for stable instead of changing too many things?~~ misread your comment. Separate PR for stable: #22085

@FRidh, kargo imports amsible. Checking kargo after the change, the script runs and the import path gets set correctly.

FRidh · 2017-01-24T09:17:00Z

@FRidh, kargo imports amsible. Checking kargo after the change, the script runs and the import path gets set correctly.

Exactly, so it is used as a library. I think that was also the reason why I put ansible in python-packages.nix, or at least, called it via python-packages.nix.

peterhoeg · 2017-01-24T10:21:01Z

Exactly, so it is used as a library. I think that was also the reason why I put ansible in python-packages.nix, or at least, called it via python-packages.nix.

Fair enough. Maybe we should have a standard rule for this (further to @jgeerds comment) documented in the manual. My take on the current situation is:

If a piece of python software is mainly invoked on its own, it should go into its own file and be referenced from from all-packages.nix and use buildPythonApplication using a preferred python version (python2Packages vs python3Packages).

If instead it is primarily used as a dependency for another piece of python software, it should go into python-packages.nix and use buildPythonPackage to allow use with multiple python versions.

Everybody in agreement?

FRidh · 2017-01-24T10:26:14Z

@peterhoeg the problem is that as soon as you put it outside of python-packages.nix it is only available for a single Python interpreter version. We don't want to have libraries in all-packages.nix again, since that caused a lot of problems in the past. Therefore, I use the following rule myself:

If it is supposed to be usable as a library, call it via python-packages.nix and use buildPythonPackage. Otherwise, put it elsewhere and use buildPythonApplication.

I don't know whether ansible is supposed to be usable as a library, but clearly it is used like that.

Anyway, buildPythonPackage should have an app attribute. Either with #16672 or an app attribute that returns a derivation but with the python prefix removed from the name as is done with buildPythonApplication.

peterhoeg · 2017-01-24T10:41:20Z

@FRidh, can't most python packages be used as libraries? In any case, I'll change that part so it goes back to the python-packages.nix but uses the generric builder instead.

fpletz · 2017-01-24T10:53:47Z

Ansible upstream won't patch the 1.x versions because it's too complicated. These versions should be removed from master and 16.09 (or at least mark at broken for 16.09).

globin · 2017-01-24T10:54:10Z

Python applications should IMHO be referenced from python-packages.nix and then be aliased in all-packages.nix with e.g ansible = python3Packages.ansible

FRidh · 2017-01-24T11:31:31Z

@FRidh, can't most python packages be used as libraries? In any case, I'll change that part so it goes back to the python-packages.nix but uses the generric builder instead.

If they install their modules in site-packages, then yes. But that's not a requirement for applications, only for libraries.

globin · 2017-01-24T11:35:17Z

If they install their modules in site-packages, then yes. But that's not a requirement for applications, only for libraries.

That means everything using buildPython{Package,Application}, right?

FRidh · 2017-01-24T11:40:30Z

That means everything using buildPython{Package,Application}, right?

Indeed, everything using that function puts modules in site-packages and is thus usable as a library.
( That was also the issue we had in the past with prefix PYTHONPATH instead of set PYTHONPATH; if a Python application uses a Python application you don't want to merge their PYTHONPATH. )

grahamc · 2017-01-25T01:03:39Z

@FRidh what is your call on this? Should we merge? Should we not merge?

peterhoeg · 2017-01-25T01:37:06Z

@FRidh and @grahamc, I will not have time to do it until tonight (UTC+7) when I'll make the following changes to this PR:

move the definitions back to python-packages.nix
drop ansible1
make 2.2 the default
update the documentation

Everybody cool?

The update to 2.2.1.0 has already been applied to stable.

grahamc · 2017-01-25T03:02:28Z

👍 thank you!

…

On Tue, Jan 24, 2017 at 8:37 PM Peter Hoeg ***@***.***> wrote: @FRidh <https://github.com/fridh> and @grahamc <https://github.com/grahamc>, I will not have time to do it until tonight but I'll make the following changes to this PR: - move the definitions back to python-packages.nix - drop ansible1 - make 2.2 the default - update the documentation Everybody cool? The update to 2.2.1.0 has already been applied to stable. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#22056 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAErrHTV48lnzdOz-ED65PNYpnY7SHIWks5rVqdEgaJpZM4LrB4g> .

FRidh · 2017-01-25T06:52:33Z

@peterhoeg yep, go ahead.

Just remember that whenever you change from buildPythonPackage to buildPythonApplication you change the name of the package and thus break upgrading with nix-env -u.

We do a few more things at the same time: a) version 2.1 is a supported version, so add that too b) create a generic builder for versions 2.1 and 2.2 c) change default ansible to ansible2 d) remove ansible1 and update the release notes

peterhoeg · 2017-01-26T07:07:56Z

All done @FRidh and @grahamc.

grahamc

@FRidh LGTY? If you don't leave feedback in ~24hrs I'll go ahead and merge.

FRidh · 2017-01-27T09:18:37Z

I've pushed 377b05a, 46b1ea2 and c42cfa1.

While there is convenience in having a generic function, it becomes very unflexible as soon as versions start to diverge.

Mic92 reviewed Jan 23, 2017

View reviewed changes

peterhoeg mentioned this pull request Jan 24, 2017

ansible2: 2.2.0.0 -> 2.2.1.0 #22085

Merged

7 tasks

fpletz added 1.severity: security 8.has: package (update) labels Jan 24, 2017

peterhoeg changed the title ~~ansible2: 2.2.0.0 -> 2.2.1.0~~ [WIP] ansible2: 2.2.0.0 -> 2.2.1.0 - please do not merge yet Jan 26, 2017

peterhoeg force-pushed the u/ansible branch from 234abbc to 7151067 Compare January 26, 2017 00:38

ansible2: 2.2.0.0 -> 2.2.1.0

6b39f94

We do a few more things at the same time: a) version 2.1 is a supported version, so add that too b) create a generic builder for versions 2.1 and 2.2 c) change default ansible to ansible2 d) remove ansible1 and update the release notes

peterhoeg force-pushed the u/ansible branch from 7151067 to 6b39f94 Compare January 26, 2017 07:04

peterhoeg changed the title ~~[WIP] ansible2: 2.2.0.0 -> 2.2.1.0 - please do not merge yet~~ ansible2: 2.2.0.0 -> 2.2.1.0 Jan 26, 2017

Merge branch 'master' into u/ansible

4933f2b

grahamc approved these changes Jan 27, 2017

View reviewed changes

FRidh closed this Jan 27, 2017

peterhoeg deleted the u/ansible branch February 2, 2017 14:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ansible2: 2.2.0.0 -> 2.2.1.0 #22056

ansible2: 2.2.0.0 -> 2.2.1.0 #22056

peterhoeg commented Jan 23, 2017

mention-bot commented Jan 23, 2017

Mic92 commented Jan 23, 2017

Mic92 Jan 23, 2017

grahamc Jan 24, 2017

globin Jan 24, 2017

peterhoeg Jan 24, 2017

peterhoeg Jan 24, 2017

jgeerds commented Jan 23, 2017

grahamc commented Jan 23, 2017

grahamc commented Jan 24, 2017

FRidh commented Jan 24, 2017

peterhoeg commented Jan 24, 2017 •

edited

FRidh commented Jan 24, 2017

peterhoeg commented Jan 24, 2017

FRidh commented Jan 24, 2017 •

edited

peterhoeg commented Jan 24, 2017

fpletz commented Jan 24, 2017

globin commented Jan 24, 2017

FRidh commented Jan 24, 2017 •

edited

globin commented Jan 24, 2017

FRidh commented Jan 24, 2017

grahamc commented Jan 25, 2017

peterhoeg commented Jan 25, 2017 •

edited

grahamc commented Jan 25, 2017 via email

FRidh commented Jan 25, 2017

peterhoeg commented Jan 26, 2017

grahamc left a comment

FRidh commented Jan 27, 2017

ansible2: 2.2.0.0 -> 2.2.1.0 #22056

ansible2: 2.2.0.0 -> 2.2.1.0 #22056

Conversation

peterhoeg commented Jan 23, 2017

Motivation for this change

Things done

mention-bot commented Jan 23, 2017

Mic92 commented Jan 23, 2017

Mic92 Jan 23, 2017

Choose a reason for hiding this comment

grahamc Jan 24, 2017

Choose a reason for hiding this comment

globin Jan 24, 2017

Choose a reason for hiding this comment

peterhoeg Jan 24, 2017

Choose a reason for hiding this comment

peterhoeg Jan 24, 2017

Choose a reason for hiding this comment

jgeerds commented Jan 23, 2017

grahamc commented Jan 23, 2017

grahamc commented Jan 24, 2017

FRidh commented Jan 24, 2017

peterhoeg commented Jan 24, 2017 • edited

FRidh commented Jan 24, 2017

peterhoeg commented Jan 24, 2017

FRidh commented Jan 24, 2017 • edited

peterhoeg commented Jan 24, 2017

fpletz commented Jan 24, 2017

globin commented Jan 24, 2017

FRidh commented Jan 24, 2017 • edited

globin commented Jan 24, 2017

FRidh commented Jan 24, 2017

grahamc commented Jan 25, 2017

peterhoeg commented Jan 25, 2017 • edited

grahamc commented Jan 25, 2017 via email

FRidh commented Jan 25, 2017

peterhoeg commented Jan 26, 2017

grahamc left a comment

Choose a reason for hiding this comment

FRidh commented Jan 27, 2017

peterhoeg commented Jan 24, 2017 •

edited

FRidh commented Jan 24, 2017 •

edited

FRidh commented Jan 24, 2017 •

edited

peterhoeg commented Jan 25, 2017 •

edited