Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set default_flow_style=None in yaml.dump calls #52957

Merged
merged 7 commits into from May 16, 2019

Conversation

@Ch3LL
Copy link
Contributor

commented May 9, 2019

What does this PR do?

In PyYaml 5.1 they changed the default_flow_style from None to False. Changing this back to None in salt since this is a breaking change to keep backwards compatibility. Here is the PR/commit that added this: yaml/pyyaml#256 yaml/pyyaml@507a464

Would appreciate any further review as I'm not sure if we want to keep backwards compatibility or update salt to ensure default_flow_style=False works properly. But I think this is the least obtrusive change for now.

What issues does this PR fix or reference?

Fixes up some tests on Fedora which has PyYaml 5.1 installed:

#52930
#52929
#52928
#52927

PyYAML 5.1:

>>> test = {'foo': 'bar'}
>>> yaml.dump(test)
'foo: bar\n'

PyYAML 3.13:

>>> test = {'food': 'bar'}
>>> yaml.dump(test)
'{food: bar}\n'

With this change it will always dump like so:

PyYAML 5.1:

>>> yaml.dump(test, default_flow_style=None)
'{food: bar}\n'

PyYAML 3.13:

>>> yaml.dump(test, default_flow_style=None)
'{food: bar}\n'

Tests written?

Yes

Commits signed with GPG?

Yes

tests/unit/utils/test_yamldumper.py Outdated Show resolved Hide resolved
Ch3LL and others added 2 commits May 9, 2019
Co-Authored-By: Wayne Werner <waynejwerner@gmail.com>
@waynew
waynew approved these changes May 9, 2019
@waynew

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

I bet there's a unicode issue on py3. When we're using pytest this assertion style will actually give us information 😄 Of course, right now... 😢

You're probably aware of both these options, but you could do
self.assertEqual(salt.utils.yamldumper.dump(data), '{!!python/unicode \'foo\': !!python/unicode \'bar\'}\n') for now. Or you could do

actual = salt.utils.yamldumper.dump(data)
assert actual == '{!!python/unicode \'foo\': !!python/unicode \'bar\'}\n', actual

to see what the actual text is.

Ch3LL added 2 commits May 10, 2019
@Ch3LL

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

i'm fine with just sticking to just assert. I know we do not get more information, but this will make it an easier transition to pytest and will get more information then.

@Ch3LL Ch3LL requested a review from saltstack/team-core May 10, 2019
@codecov

This comment has been minimized.

Copy link

commented May 13, 2019

Codecov Report

Merging #52957 into 2019.2.1 will increase coverage by 7.54%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##           2019.2.1   #52957      +/-   ##
============================================
+ Coverage     28.78%   36.32%   +7.54%     
============================================
  Files          1563     1576      +13     
  Lines        267054   269470    +2416     
  Branches      57953    56051    -1902     
============================================
+ Hits          76862    97896   +21034     
+ Misses       189762   160525   -29237     
- Partials        430    11049   +10619
Flag Coverage Δ
#arch ?
#centos7 ?
#fedora29 36.32% <100%> (?)
#proxy ?
#py2 36.32% <100%> (+7.63%) ⬆️
#py3 ?
#ubuntu1604 ?
Impacted Files Coverage Δ
salt/utils/yamldumper.py 86.27% <100%> (+10.76%) ⬆️
salt/serializers/yaml.py 77.38% <100%> (+7.5%) ⬆️
salt/serializers/yamlex.py 75% <100%> (+64.71%) ⬆️
salt/transport/frame.py 27.27% <0%> (-51.52%) ⬇️
salt/ext/ssl_match_hostname.py 0% <0%> (-36.18%) ⬇️
salt/output/virt_query.py 12.9% <0%> (-35.49%) ⬇️
salt/utils/stringio.py 52.38% <0%> (-33.34%) ⬇️
salt/modules/pdbedit.py 18.33% <0%> (-31.67%) ⬇️
salt/utils/pkg/deb.py 31.57% <0%> (-31.58%) ⬇️
salt/modules/pacmanpkg.py 25% <0%> (-31.44%) ⬇️
... and 1538 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ee05da5...e317186. Read the comment docs.

@codecov

This comment has been minimized.

Copy link

commented May 13, 2019

Codecov Report

Merging #52957 into 2019.2.1 will increase coverage by 6.88%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##           2019.2.1   #52957      +/-   ##
============================================
+ Coverage     29.78%   36.66%   +6.88%     
============================================
  Files          1574     1577       +3     
  Lines        269397   269540     +143     
  Branches      58069    57477     -592     
============================================
+ Hits          80227    98832   +18605     
+ Misses       188741   159768   -28973     
- Partials        429    10940   +10511
Flag Coverage Δ
#arch ?
#centos7 ?
#fedora29 36.66% <100%> (?)
#proxy ?
#py2 36.32% <100%> (+6.63%) ⬆️
#py3 35.88% <100%> (+26.12%) ⬆️
#ubuntu1604 ?
Impacted Files Coverage Δ
salt/utils/yamldumper.py 86.27% <100%> (+10.76%) ⬆️
salt/serializers/yaml.py 78.57% <100%> (+8.69%) ⬆️
salt/serializers/yamlex.py 78.97% <100%> (+68.69%) ⬆️
salt/ext/ssl_match_hostname.py 0% <0%> (-36.18%) ⬇️
salt/output/virt_query.py 12.9% <0%> (-35.49%) ⬇️
salt/modules/pdbedit.py 18.33% <0%> (-31.67%) ⬇️
salt/utils/pkg/deb.py 31.57% <0%> (-31.58%) ⬇️
salt/modules/pacmanpkg.py 25% <0%> (-31.44%) ⬇️
salt/runners/bgp.py 21.48% <0%> (-31.41%) ⬇️
salt/modules/bcache.py 8.5% <0%> (-31.33%) ⬇️
... and 1518 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f410346...5f6581a. Read the comment docs.

@Ch3LL Ch3LL merged commit bd02ea6 into saltstack:2019.2.1 May 16, 2019
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/jenkins/pr-merge This commit is being built
Details
WIP Ready for review
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.