aws / aws-step-functions-data-science-sdk-python

Step Functions Data Science SDK for building machine learning (ML) workflows and pipelines on AWS
Apache License 2.0
288 stars 88 forks source link

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

Closed ca-nguyen closed 3 years ago

ca-nguyen commented 3 years ago

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

Documentation

Title and description


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

StepFunctions-Bot commented 3 years ago

AWS CodeBuild CI Report

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

shivlaks commented 3 years ago

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 commented 3 years ago

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