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

salt.serializers.yaml/yamlex: remove invalid multi_constructor #47568

Merged
merged 2 commits into from May 10, 2018

Conversation

Projects
None yet
3 participants
@terminalmage
Member

terminalmage commented May 9, 2018

The first argument to add_multi_constructor must be a YAML tag. This is because when PyYAML looks for a constructor to match the tag (in order to get the function to use for deserialization) it matches the tag using .startswith(). Thus, when this is attempted using a constructor with a tag value of None, an error is raised.

The ordering of the list of multi_constructors in the Loader object appears to differ from platform to platform, which explains why this only caused the unit tests to fail on one or two platforms. If a tag match is found, then PyYAML stops iterating through the list of multi_constructors, so this invalid one has been able to stay in Salt without causing any noticeable trouble until now.

Additionally, this PR adds some exception logging to aid in troubleshooting, since we re-raise our own exception classes using the caught exception message, which means we lose the context of what actually raised the original exception.

Refs: saltstack/salt-jenkins#971

terminalmage added some commits May 9, 2018

salt.serializers.yaml/yamlex: remove invalid multi_constructor
The first argument to add_multi_constructor must be a YAML tag. This is
because when PyYAML looks for a constructor to match the tag (in order
to get the function to use for deserialization) it matches the tag using
`.startswith()`. Thus, when this is attempted using a constructor with a
tag value of `None`, an error is raised.

The ordering of the list of multi_constructors in the Loader object
appears to differ from platform to platform, which explains why this
only caused the unit tests to fail on one or two platforms. If a tag
match is found, then PyYAML stops iterating through the list of
multi_constructors, so this invalid one has been able to stay in Salt
without causing any noticeable trouble until now.
Add exception logging on serialize/deserialize exceptions
Since we are reraising an error using our own exception classes, we lose
the traceback. This adds exception logging to provide useful information
to troubleshoot errors encountered while serializing/deserializing.
@cachedout

Sheesh. Amazing find.

@cachedout

This comment has been minimized.

Contributor

cachedout commented May 9, 2018

Hrm .That failure of a core grains test is odd.

@rallytime

This comment has been minimized.

Contributor

rallytime commented May 10, 2018

@cachedout That grains test is failing on the branch and should be fixed with #47569.

@rallytime rallytime merged commit 2fcb108 into saltstack:2018.3 May 10, 2018

4 of 9 checks passed

jenkins/PR/salt-pr-linode-cent7-py3 Pull Requests » Salt PR - Linode CentOS 7 - PY3 #4795 — ABORTED
Details
jenkins/PR/salt-pr-linode-ubuntu16-py3 Pull Requests » Salt PR - Linode Ubuntu16.04 - PY3 #9764 — ABORTED
Details
jenkins/PR/salt-pr-rs-cent7-n Pull Requests » Salt PR - RS CentOS 7 #18850 — ABORTED
Details
default Build finished.
Details
jenkins/PR/salt-pr-linode-ubuntu14-n Pull Requests » Salt PR - Linode Ubuntu14.04 #22726 — FAILURE
Details
WIP ready for review
Details
jenkins/PR/salt-pr-clone Pull Requests » Salt PR - Clone #24970 — SUCCESS
Details
jenkins/PR/salt-pr-docs-n Pull Requests » Salt PR - Docs #17088 — SUCCESS
Details
jenkins/PR/salt-pr-lint-n Pull Requests » Salt PR - Code Lint #21708 — SUCCESS
Details

@terminalmage terminalmage deleted the terminalmage:salt-jenkins-971 branch May 10, 2018

rallytime added a commit that referenced this pull request May 10, 2018

rallytime added a commit that referenced this pull request May 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment