Skip to content
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

fix non-idempotent Auth@Edge function deploys #464

Merged
merged 1 commit into from Sep 25, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Expand Up @@ -5,6 +5,8 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/)
and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).

## [Unreleased]
### Fixed
- Static Site Auth@Edge: fix non-idempotent deploys when deploying from different systems

## [1.14.0] - 2020-09-23
### Changed
Expand Down
23 changes: 22 additions & 1 deletion runway/cfngin/hooks/aws_lambda.py
Expand Up @@ -3,6 +3,7 @@
import json
import logging
import os
import shutil
import stat
import subprocess
import sys
Expand Down Expand Up @@ -455,12 +456,17 @@ def dockerized_pip(

LOGGER.info('using docker image "%s" to build deployment package...', docker_image)

docker_run_args = {}
if _kwargs.get("python_dontwritebytecode"):
Copy link
Contributor

Choose a reason for hiding this comment

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

since kwargs is being used now, the proceeding underscore should be removed as it indicates an unused variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Legit embarrassing that I copied and pasted and didn't even think about that.

docker_run_args["environment"] = "1"

container = client.containers.run(
image=docker_image,
command=["/bin/sh", "-c", pip_cmd],
auto_remove=True,
detach=True,
mounts=[work_dir_mount],
**docker_run_args
)

# 'stream' creates a blocking generator that allows for real-time logs.
Expand Down Expand Up @@ -582,6 +588,10 @@ def _zip_package(
"--no-color",
]

subprocess_args = {}
if kwargs.get("python_dontwritebytecode"):
Copy link
Contributor

Choose a reason for hiding this comment

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

we could instead expose this as an env_vars/environ argument for more flexibility. I don't think it would hurt to have something like this set throughout all of the hook's actions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're probably right. I'm pretty torn on it between an implementation detail to hide away vs a configuration option.

Would be really nice if there was reasonable assurance that it wouldn't break things to just bake it in by default; I'm not certain what the implications with different types of packages/platforms.

Copy link
Contributor

Choose a reason for hiding this comment

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

just bake it in by default; I'm not certain what the implications

I'm not sure either. But, I can guarantee you someone will fine some package that will be incompatible.

We could still implement it as an environment variables option but, pre-populate it with values (like this one) by default and allow users to override the default by specifying their own to provide an out if it does break things.

Copy link
Contributor

Choose a reason for hiding this comment

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

This was added to get rid of the .pyc files correct? The slim option I mentioned and linked above looks to do this by just deleting the files rather than using the env var - .**/*.py[c|o], **/__pycache__*, and **/*.dist-info* are the default globs.

subprocess_args["env"] = dict(os.environ, PYTHONDONTWRITEBYTECODE="1")

# Pyinstaller build or explicit python path
if getattr(sys, "frozen", False) and not python_path:
script_contents = os.linesep.join(
Expand Down Expand Up @@ -612,13 +622,24 @@ def _zip_package(
)

try:
subprocess.check_call(cmd)
subprocess.check_call(cmd, **subprocess_args)
except subprocess.CalledProcessError:
raise PipError
finally:
if tmp_script.is_file():
tmp_script.unlink()

if kwargs.get("python_exclude_bin_dir") and os.path.isdir(
os.path.join(tmpdir, "bin")
):
LOGGER.debug("Removing python /bin directory from Lambda files")
shutil.rmtree(os.path.join(tmpdir, "bin"))
if kwargs.get("python_exclude_setuptools_dirs"):
for i in os.listdir(tmpdir):
if i.endswith(".egg-info") or i.endswith(".dist-info"):
LOGGER.debug("Removing directory %s from Lambda files", i)
shutil.rmtree(os.path.join(tmpdir, i))

req_files = _find_files(tmpdir, includes="**", follow_symlinks=False)
return _zip_files(req_files, tmpdir)

Expand Down
9 changes: 8 additions & 1 deletion runway/hooks/staticsite/auth_at_edge/lambda_config.py
Expand Up @@ -112,7 +112,14 @@ def write(
context,
provider,
bucket=kwargs["bucket"],
functions={handler: {"path": dirpath}},
functions={
handler: {
"path": dirpath,
"python_dontwritebytecode": True,
"python_exclude_bin_dir": True,
"python_exclude_setuptools_dirs": True,
}
},
)

# Add the lambda code reference to our context_dict
Expand Down