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.

Sign up for GitHub

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

Jump to bottom

Adding fallback support for pyyaml. #3383

Merged
merged 3 commits into from Feb 17, 2017
Merged

Adding fallback support for pyyaml. #3383

merged 3 commits into from Feb 17, 2017

Conversation

kwoodson
Copy link
Contributor

No description provided.

@sdodson
Copy link
Member

sdodson commented Feb 16, 2017

aos-ci-test

@openshift-bot
Copy link

e0988c8 - State: success - All Test Contexts: aos-ci-jenkins/OS_unit_tests - Logs: https://aos-ci.s3.amazonaws.com/openshift/openshift-ansible/jenkins-openshift-ansible-2-unit-tests-943/e0988c8eeebc9d0e9cc454058fcc2899f67ce015.txt

@kwoodson kwoodson self-assigned this Feb 16, 2017
@ashcrow
Copy link
Member

ashcrow commented Feb 16, 2017

Reviewing now.

@openshift-bot
Copy link

e0988c8 - State: success - All Test Contexts: "aos-ci-jenkins/OS_3.4_NOT_containerized, aos-ci-jenkins/OS_3.4_NOT_containerized_e2e_tests" - Logs: https://aos-ci.s3.amazonaws.com/openshift/openshift-ansible/jenkins-openshift-ansible-3-test-matrix-CONTAINERIZED=_NOT_containerized,OSE_VER=3.4,PYTHON=System-CPython-2.7,TOPOLOGY=openshift-cluster,TargetBranch=master,nodes=openshift-ansible-slave-948/e0988c8eeebc9d0e9cc454058fcc2899f67ce015.txt

@openshift-bot
Copy link

e0988c8 - State: success - All Test Contexts: "aos-ci-jenkins/OS_3.4_containerized, aos-ci-jenkins/OS_3.4_containerized_e2e_tests" - Logs: https://aos-ci.s3.amazonaws.com/openshift/openshift-ansible/jenkins-openshift-ansible-3-test-matrix-CONTAINERIZED=_containerized,OSE_VER=3.4,PYTHON=System-CPython-2.7,TOPOLOGY=openshift-cluster-containerized,TargetBranch=master,nodes=openshift-ansible-slave-948/e0988c8eeebc9d0e9cc454058fcc2899f67ce015.txt

ashcrow
ashcrow requested changes Feb 16, 2017
View reviewed changes
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.

I have one nit and one question. Overall it makes sense. Of course there are many ways to implement dual library usage but this should get the job done just fine.

roles/lib_utils/library/repoquery.py Outdated
try:
import ruamel.yaml as yaml # noqa: F401
except ImportError:
import yaml
Copy link
Member

Choose a reason for hiding this comment

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

👍

roles/lib_openshift/library/oadm_manage_node.py Outdated
if hasattr(yaml, 'RoundTripLoader'):
self.yaml_dict = yaml.load(contents, yaml.RoundTripLoader)
else:
self.yaml_dict = yaml.load(contents)
Copy link
Member

Choose a reason for hiding this comment

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

nit: safe_load. Won't block on it though.

roles/lib_openshift/library/oadm_manage_node.py Outdated
@@ -334,7 +334,11 @@ def write(self):
if hasattr(self.yaml_dict, 'fa'):
self.yaml_dict.fa.set_block_style()

Yedit._write(self.filename, yaml.dump(self.yaml_dict, Dumper=yaml.RoundTripDumper))
# pylint: disable=no-member
if hasattr(yaml, 'RoundTripDumper'):
Copy link
Member

Choose a reason for hiding this comment

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

Currently these modules all import ruamel.yaml directly and will always have yaml.RoundTripDumper. Will these be updated to try/except an ImportError like reqoquery?

@openshift-bot
Copy link

e0988c8 - State: success - All Test Contexts: "aos-ci-jenkins/OS_3.5_NOT_containerized, aos-ci-jenkins/OS_3.5_NOT_containerized_e2e_tests" - Logs: https://aos-ci.s3.amazonaws.com/openshift/openshift-ansible/jenkins-openshift-ansible-3-test-matrix-CONTAINERIZED=_NOT_containerized,OSE_VER=3.5,PYTHON=System-CPython-2.7,TOPOLOGY=openshift-cluster,TargetBranch=master,nodes=openshift-ansible-slave-948/e0988c8eeebc9d0e9cc454058fcc2899f67ce015.txt

@openshift-bot
Copy link

e0988c8 - State: success - All Test Contexts: "aos-ci-jenkins/OS_3.5_containerized, aos-ci-jenkins/OS_3.5_containerized_e2e_tests" - Logs: https://aos-ci.s3.amazonaws.com/openshift/openshift-ansible/jenkins-openshift-ansible-3-test-matrix-CONTAINERIZED=_containerized,OSE_VER=3.5,PYTHON=System-CPython-2.7,TOPOLOGY=openshift-cluster-containerized,TargetBranch=master,nodes=openshift-ansible-slave-948/e0988c8eeebc9d0e9cc454058fcc2899f67ce015.txt

@ashcrow
Copy link
Member

ashcrow commented Feb 16, 2017

aos-ci-test

@ashcrow
Copy link
Member

ashcrow commented Feb 16, 2017

@kwoodson 👍 to the changes. Assuming CI passes I say LGTM.

@openshift-bot
Copy link

c4e712b - State: success - All Test Contexts: aos-ci-jenkins/OS_unit_tests - Logs: https://aos-ci.s3.amazonaws.com/openshift/openshift-ansible/jenkins-openshift-ansible-2-unit-tests-947/c4e712ba3cc40d1deed87f56c71b337193597422.txt

@openshift-bot
Copy link

c4e712b - State: success - All Test Contexts: "aos-ci-jenkins/OS_3.4_NOT_containerized, aos-ci-jenkins/OS_3.4_NOT_containerized_e2e_tests" - Logs: https://aos-ci.s3.amazonaws.com/openshift/openshift-ansible/jenkins-openshift-ansible-3-test-matrix-CONTAINERIZED=_NOT_containerized,OSE_VER=3.4,PYTHON=System-CPython-2.7,TOPOLOGY=openshift-cluster,TargetBranch=master,nodes=openshift-ansible-slave-952/c4e712ba3cc40d1deed87f56c71b337193597422.txt

@openshift-bot
Copy link

c4e712b - State: success - All Test Contexts: "aos-ci-jenkins/OS_3.4_containerized, aos-ci-jenkins/OS_3.4_containerized_e2e_tests" - Logs: https://aos-ci.s3.amazonaws.com/openshift/openshift-ansible/jenkins-openshift-ansible-3-test-matrix-CONTAINERIZED=_containerized,OSE_VER=3.4,PYTHON=System-CPython-2.7,TOPOLOGY=openshift-cluster-containerized,TargetBranch=master,nodes=openshift-ansible-slave-952/c4e712ba3cc40d1deed87f56c71b337193597422.txt

@openshift-bot
Copy link

c4e712b - State: success - All Test Contexts: "aos-ci-jenkins/OS_3.5_NOT_containerized, aos-ci-jenkins/OS_3.5_NOT_containerized_e2e_tests" - Logs: https://aos-ci.s3.amazonaws.com/openshift/openshift-ansible/jenkins-openshift-ansible-3-test-matrix-CONTAINERIZED=_NOT_containerized,OSE_VER=3.5,PYTHON=System-CPython-2.7,TOPOLOGY=openshift-cluster,TargetBranch=master,nodes=openshift-ansible-slave-952/c4e712ba3cc40d1deed87f56c71b337193597422.txt

@openshift-bot
Copy link

c4e712b - State: success - All Test Contexts: "aos-ci-jenkins/OS_3.5_containerized, aos-ci-jenkins/OS_3.5_containerized_e2e_tests" - Logs: https://aos-ci.s3.amazonaws.com/openshift/openshift-ansible/jenkins-openshift-ansible-3-test-matrix-CONTAINERIZED=_containerized,OSE_VER=3.5,PYTHON=System-CPython-2.7,TOPOLOGY=openshift-cluster-containerized,TargetBranch=master,nodes=openshift-ansible-slave-952/c4e712ba3cc40d1deed87f56c71b337193597422.txt

@sdodson sdodson merged commit 43659a5 into openshift:master Feb 17, 2017
@sdodson sdodson mentioned this pull request Feb 17, 2017
Closed
@chrrrles
Copy link

Thanks @kwoodson - @sdodson - Do you feel that the workaround in #3383 will be present in future versions of openshift-ansible?

@kwoodson kwoodson deleted the yedit_yaml_support branch March 10, 2017 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ashcrow ashcrow ashcrow approved these changes

Assignees

@kwoodson kwoodson

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@kwoodson @sdodson @openshift-bot @ashcrow @chrrrles