support PyYAML up to 5.x version #6623

GeyseR · 2019-03-31T05:56:01Z

Resolves #6619
The yaml/pyyaml#265 ticket in the original PyYAML mention several backward-incompatible changes in v.5.1:

Deprecate yaml.load and add FullLoader and UnsafeLoader classes. docker-compose uses save yaml files parser, so it should be pretty safe to update to the new version.
Make default_flow_style=False. The only place in the docker-compose project where dump is used already set default_flow_style argument to False

Also, in the same ticket author suggests waiting for the next release if the project uses yaml.add_constructor, YAMLObject, from_yaml, to_yaml, which seems unrelated to the docker-compose project.

GordonTheTurtle · 2019-03-31T05:56:04Z

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "yaml_upgrade" [email protected]:GeyseR/compose.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

GeyseR · 2019-04-11T07:31:07Z

Any thoughts?

chris-crone · 2019-04-11T10:59:57Z

Hi @GeyseR!

Thanks for the PR. As @ijc said in the issue, it would be good to wait for pyyaml 5.2 because of known issues. When that's out, can you please update this PR?

GeyseR · 2019-04-11T11:41:01Z

Do you think those issues are critical for the compose project? In the PR description, I've added an explanation of why we can allow 5.1 without any breakage of the compose functionality.

chris-crone · 2019-04-12T11:26:51Z

Taking a more in depth look:

Deprecation of yaml.load

We use this in a couple of places so we should move to yaml.safe_load in the PR that bumps pyyaml.

Also the logic for detecting special characters was broken, which results in strings with tabs (for example) to be dumped in plain style on python 2.7 on systems with sys.maxunicode <= 0xffff.

That is worrying as we support Python 2.7.

Thank you for this PR and for taking the time to investigate this properly. Given the Python 2.7 issue though, I think it's best to hold off until pyyaml 5.2.

GeyseR · 2019-04-12T12:19:40Z

We use this in a couple of places so we should move to yaml.safe_load in the PR that bumps pyyaml.
i've updated all calls to yaml.load to yaml.save_load. All those places were in tests, the actual package code already uses yaml.safe_load/yaml.safe_dump everywhere.

That is worrying as we support Python 2.7.
yea, I didn't take it into account while I probably had to. I would agree that we should wait for the pyyaml v.5.2. Do you think we should keep this PR and set it On Hold, or open a new one when 5.2 will be released? (In case of the first option I can take care of updating existing PR according to the changes in the new pyyaml release)

chris-crone · 2019-04-12T12:35:04Z

i've updated all calls to yaml.load to yaml.save_load. All those places were in tests, the actual package code already uses yaml.safe_load/yaml.safe_dump everywhere.

Thank you!

yea, I didn't take it into account while I probably had to. I would agree that we should wait for the pyyaml v.5.2. Do you think we should keep this PR and set it On Hold, or open a new one when 5.2 will be released? (In case of the first option I can take care of updating existing PR according to the changes in the new pyyaml release)

Since you've done the work of moving to safe_load, we can leave this open and wait for 5.2. It's useful to have this as we have the CI runs with the changes so we can see if they break anything.

Fixes security issues with Jinja2, urllib3, pyyaml. Raises an error when installing pyyaml==5.1 due to pinned constraints in docker-compose and aws-cli. AWS-cli has issues w pyyaml 5 dropping support for python 2.6 aws/aws-cli#3828 Docker compose is still waiting on some issues w unicode support in pyyaml 5.1, but this doesn't affect us. docker/compose#6623

richardhanson · 2019-05-17T17:39:34Z

Unless I'm missing something, the Python 2 issue is not relevant here. Since the package code already uses safe_load everywhere and this is just a dependency and test update, can this be merged? I'm working with a pytest plugin that is affected by this.

Thank you for being cautious and careful!

EDIT:
Nevermind on the whole thing. I didn't understand that 5.1 broke existing functionality in dump for Python 2.7. Can't wait for 1/1/2020!

graingert · 2019-05-20T09:13:11Z

That is worrying as we support Python 2.7.

although that seems to only be broken in yaml.dump which isn't used, except in an example script which uses ruamel.yaml.dump

compose/contrib/migration/migrate-compose-file-v1-to-v2.py

Lines 136 to 142 in 75d5eb0

    
           ruamel.yaml.dump( 
        
               new_format, 
        
               stream, 
        
               Dumper=ruamel.yaml.RoundTripDumper, 
        
               indent=indent, 
        
               width=width)

graingert · 2019-05-20T09:14:15Z

ah, no there's a dump here:

compose/compose/config/serialize.py

Lines 105 to 112 in 75d5eb0

    
           return yaml.safe_dump( 
        
               denormalize_config(config, image_digests), 
        
               default_flow_style=False, 
        
               indent=2, 
        
               width=80, 
        
               allow_unicode=True 
        
           )

hartwork · 2019-06-08T23:01:08Z

@GeyseR can you rebase this against latest master to address the conflicts?

My vote for getting this into 1.25 (rather than 1.26).

hartwork · 2019-06-08T23:03:56Z

There is one more call to .load that might be worth checking, this time with ruamel, though:

--- a/contrib/migration/migrate-compose-file-v1-to-v2.py
+++ b/contrib/migration/migrate-compose-file-v1-to-v2.py
@@ -19,7 +19,7 @@ log = logging.getLogger('migrate')
 
 
 def migrate(content):
-    data = ruamel.yaml.load(content, ruamel.yaml.RoundTripLoader)
+    data = ruamel.yaml.safe_load(content, ruamel.yaml.RoundTripLoader)
 
     service_names = data.keys()

GeyseR · 2019-06-09T10:55:59Z

Hey @hartwork
Thanks for looking into this issue and PR. I definitely support your opinion about releasing this as part of 1.25 release. I've rebased my changed against current master.
Unfortunately, the pyyaml team hasn't released 5.2 version yet, so it looks like there could be some incompatibility problems with Python 2.7

There is one more call to .load that might be worth checking, this time with ruamel

compose uses ruamel's RoundTripLoader for loading files, that uses RoundTripConstructor that in turn inherits from SafeConstructor. So we should be ok there.

GeyseR · 2019-10-23T08:26:34Z

rebased on current master

graingert · 2019-12-09T16:18:55Z

@rumpl will this make it for 1.25.1? (eg py2 supported)

graingert · 2019-12-13T13:38:17Z

@ulyssessouza how many reviewers are required for a PR?

ulyssessouza · 2019-12-13T13:54:26Z

@graingert A PR normally needs the approval of 2 maintainers. This PR will get merged just after 1.25.1 is released.

The reason is that 1.25.1-rc1 is on testing phase to be promoted to 1.25.1 and as this is a major version change, that would be better to have this tested in an RC before.
This does not explain why it's not merged. And this is because the release process is quite tricky, so we prefer to have a clean master branch just to have a smooth release.
This should change for 2020 thanks to the work of @ndeloof in #7079

I'm very sorry to have a PR that LGTM not merged for that long but we are working on "cleaning the house" to get better workflows.

graingert · 2019-12-13T13:58:47Z

@ulyssessouza will it make a 1.25.x release? Eg py2 still supported?

ulyssessouza · 2019-12-13T14:10:03Z

@graingert Probably not

kohtala · 2019-12-19T07:18:29Z

So Python3 would work with PyYAML 5.x? Would it help to use environment markers and provide different PyYAML requirement for python 2.7 and 3.x?

So in setup.py

'PyYAML >= 3.10, < 5',

would be like

'PyYAML >= 3.10, < 5; python_version == "2.7"',
'PyYAML >= 3.10, < 6; python_version != "2.7"',

There are other choises of comparisons, but that should work for existing and future versions.

I am a little impatient as I am fighting now to use two libraries at the same time where other requires PyYAML 5. I regret moving to Pipenv as I find no way to tell it to ignore this requires-dist that does not affect our use.

graingert · 2020-01-08T09:30:32Z

So Python3 would work with PyYAML 5.x? Would it help to use environment markers and provide different PyYAML requirement for python 2.7 and 3.x?

So in setup.py
'PyYAML >= 3.10, < 5',
would be like
'PyYAML >= 3.10, < 5; python_version == "2.7"',
'PyYAML >= 3.10, < 6; python_version != "2.7"',
There are other choises of comparisons, but that should work for existing and future versions.

I am a little impatient as I am fighting now to use two libraries at the same time where other requires PyYAML 5. I regret moving to Pipenv as I find no way to tell it to ignore this requires-dist that does not affect our use.

There would be no point in doing that because this PR provides pyaml 3.10 to 6 support for both py2 and py3

ulyssessouza · 2020-01-08T09:54:18Z

Python2 will be dropped in version 1.26.0 anyways. Check #7031

graingert · 2020-01-08T10:08:53Z

Python2 will be dropped in version 1.26.0 anyways. Check #7031

This pr is compatible with a v1.25.x release though

requirements.txt

ulyssessouza · 2020-01-08T12:43:02Z

Just squash the commits and that's good for me

graingert · 2020-01-08T15:03:26Z

@GeyseR this will also need a rebase to deal with the conflicts in requirements.txt

@ulyssessouza if you merge PRs that manually change requirements.txt and therefore conflict with dependabot PRs dependabot will automatically rebase to fix the conflicts, this way you don't have to wait for a human to resolve the conflicts

kohtala · 2020-01-09T07:58:15Z

There would be no point in doing that because this PR provides pyaml 3.10 to 6 support for both py2 and py3

I know. I do not disrespects this PR. If this PR is really going in now, I am happy, but I was maybe unclear in asking if another PR could go in while waiting for this.

docker-compose can not be currently installed with some other libraries. It is a blocker, and a partial solution would be better than keeping those unaffected waiting.

graingert · 2020-01-09T08:05:17Z

There would be no point in doing that because this PR provides pyaml 3.10 to 6 support for both py2 and py3

I know. I do not disrespects this PR. If this PR is really going in now, I am happy, but I was maybe unclear in asking if another PR could go in while waiting for this.

docker-compose can not be currently installed with some other libraries. It is a blocker, and a partial solution would be better than keeping those unaffected waiting.

There is no partial solution that is py3 only, the code change in this PR is required to support pyyaml~=5.2 on either py3 or py2

graingert · 2020-01-09T08:06:28Z

@GeyseR can you rebase and squash everything into 1 commit?

Signed-off-by: Sergey Fursov <[email protected]>

GeyseR · 2020-01-09T09:48:19Z

@graingert @ulyssessouza I've squashed commits and rebased changes against the actual master branch

ulyssessouza · 2020-01-09T15:50:21Z

Thank you all and specially @GeyseR for the patience!

graingert · 2020-01-09T16:18:24Z

@ulyssessouza great stuff! Do you know when the next release will be?

ulyssessouza · 2020-01-14T09:47:30Z

That will be 1.26.0 for sure but for the when, that depends on the advancement on the CI rework

graingert · 2020-01-20T17:05:28Z

@ulyssessouza I see docker-compose==1.25.2rc2 has 'PyYAML >= 3.10, < 6', and I see https://github.com/docker/compose/releases/tag/v1.25.2 but it's not on pypi yet

hartwork · 2020-01-20T18:23:16Z

It is now: https://pypi.org/project/docker-compose/1.25.2/

graingert · 2020-01-20T18:46:12Z

@hartwork awesome I'm upgrading everywhere now!

GordonTheTurtle added the dco/no label Mar 31, 2019

GordonTheTurtle removed the dco/no label Mar 31, 2019

chris-crone added this to the 1.25.0 milestone Apr 11, 2019

chris-crone requested a review from ulyssessouza April 11, 2019 10:57

chris-crone removed this from the 1.25.0 milestone Apr 11, 2019

chris-crone removed the request for review from ulyssessouza April 11, 2019 10:57

chris-crone self-requested a review April 12, 2019 12:31

chris-crone added this to the 1.25.0 milestone Apr 12, 2019

chrisgemignani mentioned this pull request Apr 24, 2019

[JB-2256] Update devlandia requirements juiceinc/devlandia#37

Closed

3 tasks

ijc added area/build kind/enhancement labels May 20, 2019

ulyssessouza modified the milestones: 1.25.0, 1.26.0 May 28, 2019

hartwork mentioned this pull request Jun 8, 2019

[W.I.P.] Use yaml.safe_load rather than yaml.load #6744

Closed

ulyssessouza mentioned this pull request Jul 8, 2019

Strip up generic versions and bump requests #6778

Merged

graingert approved these changes Dec 9, 2019

View reviewed changes

graingert mentioned this pull request Dec 9, 2019

Bump pyyaml from 4.2b1 to 5.2 #7089

Closed

graingert mentioned this pull request Dec 23, 2019

Bump pyyaml from 4.2b1 to 5.3b1 #7107

Closed

graingert mentioned this pull request Jan 8, 2020

Bump pyyaml from 4.2b1 to 5.3 #7131

Closed

ulyssessouza reviewed Jan 8, 2020

View reviewed changes

requirements.txt Outdated Show resolved Hide resolved

GeyseR changed the title ~~support PyYAML up to 5.2 version~~ support PyYAML up to 5.x version Jan 8, 2020

support PyYAML up to 5.x version

1b1823e

Signed-off-by: Sergey Fursov <[email protected]>

GordonTheTurtle removed the dco/no label Jan 9, 2020

ndeloof merged commit c818bfc into docker:master Jan 9, 2020

support PyYAML up to 5.x version #6623

support PyYAML up to 5.x version #6623

Conversation

GeyseR commented Mar 31, 2019 • edited Loading

GordonTheTurtle commented Mar 31, 2019

GeyseR commented Apr 11, 2019

chris-crone commented Apr 11, 2019

GeyseR commented Apr 11, 2019

chris-crone commented Apr 12, 2019

GeyseR commented Apr 12, 2019

chris-crone commented Apr 12, 2019

richardhanson commented May 17, 2019 • edited Loading

graingert commented May 20, 2019

graingert commented May 20, 2019

hartwork commented Jun 8, 2019

hartwork commented Jun 8, 2019

GeyseR commented Jun 9, 2019

GeyseR commented Oct 23, 2019

graingert commented Dec 9, 2019 • edited Loading

graingert commented Dec 13, 2019

ulyssessouza commented Dec 13, 2019

graingert commented Dec 13, 2019

ulyssessouza commented Dec 13, 2019

kohtala commented Dec 19, 2019

graingert commented Jan 8, 2020

ulyssessouza commented Jan 8, 2020

graingert commented Jan 8, 2020

ulyssessouza commented Jan 8, 2020

graingert commented Jan 8, 2020 • edited Loading

kohtala commented Jan 9, 2020

graingert commented Jan 9, 2020

graingert commented Jan 9, 2020

GeyseR commented Jan 9, 2020

ulyssessouza commented Jan 9, 2020

graingert commented Jan 9, 2020

ulyssessouza commented Jan 14, 2020

graingert commented Jan 20, 2020

hartwork commented Jan 20, 2020

graingert commented Jan 20, 2020

GeyseR commented Mar 31, 2019 •

edited

Loading

richardhanson commented May 17, 2019 •

edited

Loading

graingert commented Dec 9, 2019 •

edited

Loading

graingert commented Jan 8, 2020 •

edited

Loading