Implementation of "sam build" CLI command#766
Conversation
jfuss
left a comment
jfuss
left a comment
There was a problem hiding this comment.
This looks really good! Super excited for this capability to be added!!!!!! 🎉🎉🎉🎉🎉
| template): | ||
| # All logic must be implemented in the ``do_cli`` method. This helps with easy unit testing | ||
|
|
||
| do_cli(template, source_root, build_dir, clean, native) # pragma: no cover |
There was a problem hiding this comment.
nit: can we match param order between cli and do_cli methods
jfuss
left a comment
jfuss
left a comment
There was a problem hiding this comment.
Shaping up!
Don't forget about docstrings and you should really use Pathlib over os.path. I find it much easier to read and understand when doing Path manipulations.
PS: We really need that class to encapsulate all the Path madness.
| import click | ||
|
|
||
| _TEMPLATE_OPTION_DEFAULT_VALUE = "template.[yaml|yml]" | ||
| _TEMPLATE_SEARCH_PATHS = [ |
There was a problem hiding this comment.
Instead. What if you keep the default and have commands pass in the extra resolving that should take priority?
This will simplify resolving and will allow the commands to control the resolve order. For example, package should only care about build and deploy should only care about package (templates). If they don't exist then some other flow would be triggered anyways.
|
|
||
| _TEMPLATE_OPTION_DEFAULT_VALUE = "template.[yaml|yml]" | ||
| _TEMPLATE_SEARCH_PATHS = [ | ||
| os.path.join(".sam", "build", "template.yaml"), |
There was a problem hiding this comment.
Consider making this directory bound instead of template.
Also pathlib :)
| :return dict: Template data as a dictionary | ||
| :raises InvokeContextException: If template file was not found or the data was not a JSON/YAML | ||
| """ | ||
| # TODO: This method was copied from InvokeContext. Move it into a common folder |
sanathkr
changed the title
sriram-mv
left a comment
sriram-mv
left a comment
There was a problem hiding this comment.
Have some minor comments, but nothing blocking.
Excited for this! 💃
|
|
||
|
|
||
| def docker_common_options(f): | ||
| for option in reversed(docker_click_options()): |
There was a problem hiding this comment.
Q: What does reversed really give us?
There was a problem hiding this comment.
It allows us to list options in the code in the order it will appear in help text. Just for our convenience.
|
|
||
| class BuildContext(object): | ||
|
|
||
| _BUILD_DIR_PERMISSIONS = 0o755 |
There was a problem hiding this comment.
Maybe a comment on why these permissions?
| "manifest_dir": "{}/manifest".format(base) | ||
| } | ||
|
|
||
| if pathlib.PurePath(source_dir) == pathlib.PurePath(manifest_dir): |
There was a problem hiding this comment.
I spent 3 hours debugging an issue that stemmed from not having this check ;)
| process.wait() | ||
|
|
||
| process_stdout = b"".join(process.stdout.readlines()).strip().decode('utf-8') | ||
| print(process_stdout) |
There was a problem hiding this comment.
we can probably remove the prints?
There was a problem hiding this comment.
I do want to have prints for debugging. pytest prints them only if your test fails
| "optimizations", | ||
| "options") | ||
|
|
||
| self.maxDiff = None # Print whole json diff |
There was a problem hiding this comment.
No, its helpful in debugging when the diff doesn't match up
|
THANK YOU THANK YOU THANK YOU! As we just started to explore SAM, we quickly bumped into the python packaging crap. |
| help="Specify whether CLI should skip pulling down the latest Docker image for Lambda runtime.", | ||
| envvar="SAM_SKIP_PULL_IMAGE"), | ||
|
|
||
| click.option('--docker-network', |
There was a problem hiding this comment.
Is there a way to pass environment variables to the container ?
5 participants
Description of changes:
Implementation of #743 -
sam buildcommand.Implementation is complete. It includes unit and integration tests.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.