Skip to content

Conversation

@KeltonKarboviak
Copy link
Contributor

@KeltonKarboviak KeltonKarboviak commented Feb 12, 2019

Issue #, if available:

N.A.

Description of changes:

Most classes seem to follow this parameter pattern: (artifacts_dir, scratch_dir, manifest_path)

The PythonPipWorkflow class constructs the PythonPipBuildAction with this order of arguments: (artifacts_dir, scratch_dir, manifest_path, runtime)

https://github.com/awslabs/aws-lambda-builders/blob/29f254bc655c84b6ae651eee556787cb1cb5875c/aws_lambda_builders/workflows/python_pip/workflow.py#L67-L68

The PythonPipBuildAction class has the following parameter order for its constructor: (artifacts_dir, manifest_path, scratch_dir, runtime), which flips the scratch_dir & manifest_path arguments.

https://github.com/awslabs/aws-lambda-builders/blob/29f254bc655c84b6ae651eee556787cb1cb5875c/aws_lambda_builders/workflows/python_pip/actions.py#L17

The process still works because here:

https://github.com/awslabs/aws-lambda-builders/blob/29f254bc655c84b6ae651eee556787cb1cb5875c/aws_lambda_builders/workflows/python_pip/actions.py#L36-L40

the PythonPipBuildAction calls package_builder.build_dependencies(self.artifacts_dir, self.manifest_path, self.scratch_dir), even though the defined parameter order is the following: (artifacts_dir_path, scratch_dir_path, requirements_path), which again flips the scratch_dir & manifest_path arguments from the Action class.

https://github.com/awslabs/aws-lambda-builders/blob/29f254bc655c84b6ae651eee556787cb1cb5875c/aws_lambda_builders/workflows/python_pip/packager.py#L107-L108

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

@KeltonKarboviak
Copy link
Contributor Author

All python_pip workflow tests pass:

➜ pytest tests/**/python_pip/
================================================ test session starts ================================================
platform darwin -- Python 3.7.2, pytest-3.0.7, py-1.4.33, pluggy-0.4.0
rootdir: /Users/kelton.karboviak/Code/aws-lambda-builders, inifile:
plugins: cov-2.4.0
collected 79 items

tests/functional/workflows/python_pip/test_packager.py .....................................
tests/functional/workflows/python_pip/test_utils.py .
tests/integration/workflows/python_pip/test_python_pip.py .....
tests/unit/workflows/python_pip/test_actions.py ..
tests/unit/workflows/python_pip/test_packager.py .........................
tests/unit/workflows/python_pip/test_validator.py .......
tests/unit/workflows/python_pip/test_workflow.py ..

============================================ 79 passed in 187.18 seconds ============================================

@sanathkr
Copy link
Contributor

Thanks for making this change! I remember finding this once when I was working on an unrelated problem but didn’t have the bandwidth to make this change. Good find!

dependency_builder=dependency_builder)
try:
package_builder.build_dependencies(
self.artifacts_dir,
Copy link
Contributor

Choose a reason for hiding this comment

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

While we are here, could you add kwarg mapping when calling build_dependencies here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed up new commit with these changes

sriram-mv
sriram-mv previously approved these changes Feb 19, 2019
@sanathkr
Copy link
Contributor

@KeltonKarboviak Looks like the rebase didn't happen correctly and the diff shows a lot of existing code from develop. Can you fix it? Will help getting a review and merging..

@KeltonKarboviak
Copy link
Contributor Author

@sanathkr Sorry about that, I think it should be fixed now.

@sriram-mv sriram-mv self-requested a review April 16, 2019 20:06
Copy link
Contributor

@sriram-mv sriram-mv left a comment

Choose a reason for hiding this comment

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

👍

@KeltonKarboviak
Copy link
Contributor Author

@sanathkr @thesriram Is this able to be merged in now?

@sriram-mv sriram-mv merged commit 6696532 into aws:develop Apr 22, 2019
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