Skip to content

test: Correcting cloudformation export tests to use yaml.safe_load() #172

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

Merged
merged 1 commit into from
Oct 15, 2021

Conversation

ca-nguyen
Copy link
Contributor

@ca-nguyen ca-nguyen commented Oct 15, 2021

Description

PyYAML deprecated use of yaml.load() function without Loader argument since 5.1 (see deprecation doc).

Fixes failing Codebuild unit tests (see failing Codebuild logs in PR #166)

Why is the change necessary?

Unit test test_cloudformation_export_with_sagemaker_execution_role started failing on 10/14 due to upgrade of PyYAML from 5.4.41 to 6.0.0 with error:

TypeError: load() missing 1 required positional argument: 'Loader'

PyYAML introduced changes in 6.0.0 to always require Loader arg to yaml.load() (see release notes). The use of yaml.load() without Loader argument has been deprecated with warning since 5.1, but was tolerated before the breaking change in v6.0.0.

Solution

Call yaml.safe_load() instead of yaml.load() which was deemed unsafe since its release in 2006.

PyYAML has always provided a safe_load function that can load a subset of YAML without exploit.

Updated existing tests instead of freezing PyYAML to v5.4.1 because:

  1. yaml.safe_load() is safer than yaml.load() per PyYAML recommendation: it uses a SafeLoader instead of a FullLoader which allows exploits on untrusted input (see deprecation doc for more details)
  2. Requires minimal change: Only 2 tests call yaml.load()

Testing

Ran the unit tests locally and confirmed they passed.

pip install ".[test]"
tox -v tests/unit

Pull Request Checklist

Please check all boxes (including N/A items)

Testing

  • Unit tests added - N/A
  • Integration test added - N/A
  • Manual testing - why was it necessary? could it be automated?

Documentation

  • docs: All relevant docs updated - N/A
  • docstrings: All public APIs documented - N/A

Title and description

  • Change type: Title is prefixed with change type: and follows conventional commits
  • References: Indicate issues fixed via: Fixes #xxx - N/A

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.

PyYAML deprecated use of yaml.load() function without Loader argument since 5.1. Updating to call safe_load() instead as load() was deemed unsafe per PyYAML documatation since its first release in 2006
@StepFunctions-Bot
Copy link
Contributor

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-sEHrOdk7acJc
  • Commit ID: 38e0c52
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@shivlaks
Copy link
Contributor

thanks for making this fix!! please mark it ready for review when you've filled in any missing details, but LGTM.

nit: the checkboxes in your CR summary have spaces, which is why they don't render
To correct, please change them to [x]

@ca-nguyen ca-nguyen marked this pull request as ready for review October 15, 2021 16:13
@ca-nguyen
Copy link
Contributor Author

Thanks for the fast review @shivlaks !
Marking as ready for review!

@ca-nguyen ca-nguyen merged commit 5f73cf7 into aws:main Oct 15, 2021
@ca-nguyen ca-nguyen deleted the fix-cloudformation-export-tests branch October 15, 2021 16:28
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.

4 participants