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

Support unicode output when dumping yaml #3808

Merged
merged 1 commit into from Apr 4, 2017

Conversation

mtnbikenc
Copy link
Member

@mtnbikenc mtnbikenc commented Mar 30, 2017

When using allow_unicode=True in yaml.dump statements, if the output contains a unicode character it will cause the module to fail. Using six.u to properly pass through all unicode characters. This can have the side effect of passing unicode characters even if they were not intentionally included, such as non-breaking spaces in URLs. See bug 1416877.

@mtnbikenc mtnbikenc requested a review from ashcrow March 30, 2017 18:18
@mtnbikenc
Copy link
Member Author

aos-ci-test

@openshift-bot
Copy link

success: aos-ci-jenkins/OS_unit_tests for 92b21f9 (logs)

Copy link
Member

@ashcrow ashcrow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments.

@@ -657,7 +657,7 @@ def to_padded_yaml(data, level=0, indent=2, **kw):
try:
transformed = yaml.dump(data, indent=indent, allow_unicode=True,
default_flow_style=False,
Dumper=AnsibleDumper, **kw)
Dumper=AnsibleDumper, **kw).decode('utf-8')
Copy link
Member

@ashcrow ashcrow Mar 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sadly, this won't work across versions

Python 2

$ python2 -c "import yaml; print(yaml.dump({}, allow_unicode=True).decode('utf-8'))"
{}

Python 3

$ python3 -c "import yaml; print(yaml.dump({}, allow_unicode=True).decode('utf-8'))"
Traceback (most recent call last):
  File "<string>", line 1, in <module>
AttributeError: 'str' object has no attribute 'decode

Copy link
Member

@ashcrow ashcrow Mar 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A possible solution is using six.u():

$ python2 -c "import six, yaml; print(six.u(yaml.dump({}, allow_unicode=True)))
{}
$ python3 -c "import six, yaml; print(six.u(yaml.dump({}, allow_unicode=True)))
{}

@openshift-bot
Copy link

error: aos-ci-jenkins/OS_3.4_NOT_containerized for 92b21f9 (logs)

@openshift-bot
Copy link

success: "aos-ci-jenkins/OS_3.4_containerized, aos-ci-jenkins/OS_3.4_containerized_e2e_tests" for 92b21f9 (logs)

@openshift-bot
Copy link

success: "aos-ci-jenkins/OS_3.5_NOT_containerized, aos-ci-jenkins/OS_3.5_NOT_containerized_e2e_tests" for 92b21f9 (logs)

@openshift-bot
Copy link

success: "aos-ci-jenkins/OS_3.5_containerized, aos-ci-jenkins/OS_3.5_containerized_e2e_tests" for 92b21f9 (logs)

@mtnbikenc mtnbikenc changed the title Use decode when using allow_unicode Support unicode output when dumping yaml Mar 31, 2017
@mtnbikenc
Copy link
Member Author

mtnbikenc commented Mar 31, 2017

Wrapping ansible.compat.six in a try/except block because ansible.compat.six goes away with Ansible 2.4.
ansible/ansible#22855

@mtnbikenc
Copy link
Member Author

aos-ci-test

Copy link
Member

@ashcrow ashcrow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change looks good!

@ashcrow
Copy link
Member

ashcrow commented Mar 31, 2017

[test]

@openshift-bot
Copy link

success: aos-ci-jenkins/OS_unit_tests for 9a38f01 (logs)

@openshift-bot
Copy link

success: "aos-ci-jenkins/OS_3.4_NOT_containerized, aos-ci-jenkins/OS_3.4_NOT_containerized_e2e_tests" for 9a38f01 (logs)

@openshift-bot
Copy link

success: "aos-ci-jenkins/OS_3.4_containerized, aos-ci-jenkins/OS_3.4_containerized_e2e_tests" for 9a38f01 (logs)

@openshift-bot
Copy link

success: "aos-ci-jenkins/OS_3.5_NOT_containerized, aos-ci-jenkins/OS_3.5_NOT_containerized_e2e_tests" for 9a38f01 (logs)

@openshift-bot
Copy link

success: "aos-ci-jenkins/OS_3.5_containerized, aos-ci-jenkins/OS_3.5_containerized_e2e_tests" for 9a38f01 (logs)

@ashcrow
Copy link
Member

ashcrow commented Apr 3, 2017

[merge]

@ashcrow
Copy link
Member

ashcrow commented Apr 3, 2017

I have a hard time believing this is not a flake.

      the server has asked for the client to provide credentials (get builds)
  not to have occurred

  /go/src/github.com/openshift/origin/test/extended/builds/new_app.go:39

@ashcrow
Copy link
Member

ashcrow commented Apr 3, 2017

@mtnbikenc you may want to try a rebase in case this is a flake from an earlier merge.

@mtnbikenc
Copy link
Member Author

aos-ci-test

@mtnbikenc
Copy link
Member Author

[merge]

@openshift-bot
Copy link

Evaluated for openshift ansible test up to 203630f

@openshift-bot
Copy link

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_openshift_ansible_extended_conformance_install/101/) (Base Commit: 0bac74c)

@ashcrow
Copy link
Member

ashcrow commented Apr 3, 2017

device mapper flake (does not always cause a test failure) https://github.com/openshift/origin/issues/13271

@ashcrow
Copy link
Member

ashcrow commented Apr 3, 2017

aos-ci-test

@ashcrow
Copy link
Member

ashcrow commented Apr 3, 2017

[merge]

@openshift-bot
Copy link

success: aos-ci-jenkins/OS_unit_tests for 203630f (logs)

@openshift-bot
Copy link

success: "aos-ci-jenkins/OS_3.4_NOT_containerized, aos-ci-jenkins/OS_3.4_NOT_containerized_e2e_tests" for 203630f (logs)

@openshift-bot
Copy link

success: "aos-ci-jenkins/OS_3.4_containerized, aos-ci-jenkins/OS_3.4_containerized_e2e_tests" for 203630f (logs)

@openshift-bot
Copy link

success: "aos-ci-jenkins/OS_3.5_NOT_containerized, aos-ci-jenkins/OS_3.5_NOT_containerized_e2e_tests" for 203630f (logs)

@openshift-bot
Copy link

success: "aos-ci-jenkins/OS_3.5_containerized, aos-ci-jenkins/OS_3.5_containerized_e2e_tests" for 203630f (logs)

@openshift-bot
Copy link

success: aos-ci-jenkins/OS_unit_tests for 203630f (logs)

@openshift-bot
Copy link

success: "aos-ci-jenkins/OS_3.4_containerized, aos-ci-jenkins/OS_3.4_containerized_e2e_tests" for 203630f (logs)

@openshift-bot
Copy link

success: "aos-ci-jenkins/OS_3.4_NOT_containerized, aos-ci-jenkins/OS_3.4_NOT_containerized_e2e_tests" for 203630f (logs)

@openshift-bot
Copy link

success: "aos-ci-jenkins/OS_3.5_containerized, aos-ci-jenkins/OS_3.5_containerized_e2e_tests" for 203630f (logs)

@openshift-bot
Copy link

success: "aos-ci-jenkins/OS_3.5_NOT_containerized, aos-ci-jenkins/OS_3.5_NOT_containerized_e2e_tests" for 203630f (logs)

@sdodson
Copy link
Member

sdodson commented Apr 4, 2017

[merge]

@openshift-bot
Copy link

Evaluated for openshift ansible merge up to 203630f

@openshift-bot
Copy link

openshift-bot commented Apr 4, 2017

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_openshift_ansible/151/) (Base Commit: 68b3156)

@openshift-bot openshift-bot merged commit 7496b12 into openshift:master Apr 4, 2017
@mtnbikenc mtnbikenc deleted the unicode-decode branch April 4, 2017 14:34
@smarterclayton
Copy link
Contributor

This appears to have resulted in #3978 - what's the change I need to make to how my identity provider var is specified here: https://github.com/openshift/release/blob/master/cluster/test-deploy/data/vars.yaml#L39

Or alternatively, is this unrelated and just looks like it?

@ashcrow
Copy link
Member

ashcrow commented Apr 24, 2017

@smarterclayton @mtnbikenc I think we have an issue with PyYAML using dump instead of safe_dump:

In [11]: yaml.dump(u'tag:yaml.org,2002:map')
Out[11]: "!!python/unicode 'tag:yaml.org,2002:map'\n"

Out[19]: "- {!!python/unicode 'challenge': !!python/unicode 'true', !!python/unicode 'kind': !!python/unicode 'AllowAllPasswordIdentityProvider',\n  !!python/unicode 'login': !!python/unicode 'true', !!python/unicode 'name': !!python/unicode 'allow_all'}\n"

vs

In [15]: yaml.safe_dump('tag:yaml.org,2002:map')
Out[15]: 'tag:yaml.org,2002:map\n...\n'

In [20]: yaml.safe_dump(d)
Out[20]: "- {challenge: 'true', kind: AllowAllPasswordIdentityProvider, login: 'true', name: allow_all}\n"

@smarterclayton
Copy link
Contributor

Will try locally.

@smarterclayton
Copy link
Contributor

The ansible dumper doesn't have the same semantics - what would a proposed patch look like that I can try?

@ashcrow
Copy link
Member

ashcrow commented Apr 24, 2017

It looks like the ansible dumper, at least via code invocation, doesn't like Unicode by default:

In [1]: d = [{u'name': u'allow_all', u'login': u'true', u'challenge': u'true', u'kind': u'AllowAllPasswordIdentityProvider'}]

In [2]: from ansible.parsing.yaml import dumper

In [3]: dumper.yaml.dump(d)
Out[3]: "- {!!python/unicode 'challenge': !!python/unicode 'true', !!python/unicode 'kind': !!python/unicode 'AllowAllPasswordIdentityProvider',\n  !!python/unicode 'login': !!python/unicode 'true', !!python/unicode 'name': !!python/unicode 'allow_all'}\n"

But is happy with it's safe_dump

In [9]: dumper.yaml.safe_dump(d)
Out[9]: "- challenge: 'true'\n  kind: AllowAllPasswordIdentityProvider\n  login: 'true'\n  name: allow_all\n"

@mtnbikenc

  • This post shows a couple possible solutions.
  • Here is the results of calls when using PyYAML (not sure about ruamel).

I'll see if I can create a quick diff that should work for testing.

@ashcrow
Copy link
Member

ashcrow commented Apr 24, 2017

Switching over to the new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants