fix non-idempotent Auth@Edge function deploys #464
Conversation
pyrsa/pyjwt create scripts with a shebang specific to the build system. Removing them, and setuptools remnants containing various hash values, stops new functions from being built on different systems. It's possible the `PYTHONDONTWRITEBYTECODE` option is safe to apply unconditionally to all function builds by default, but I'm not 100% certain of the implications of that so I've gated it to an explicit option for now. All of the new options have been intentionally left off the docs for this first pass where we may revise the names.
|
New zip: Old zip with PYTHONDONTWRITEBYTECODE: Original zip: |
|
@ITProKyle feel free to make any changes directly or assign a revising issue to me for anything you'd prefer to be different. |
There was a problem hiding this comment.
For a first pass/patch this should be fine. Only a few minor changes i would make.
As for the deletion of bin and *-info, I would rather see it mimic serverless-python-requirements (partially to simplify moving between the two tools) like we did for the docker implementation. They use slim (default set of globs to remove) with optional extra glob patterns to give a bit more control.
slim: true
slimPatternsAppendDefaults: true
slimPatterns:
- '**/*.egg-info*'I put this all here so I could comment directly on the code. I can create issues later that link back to the comments.
| @@ -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"): | |||
There was a problem hiding this comment.
since kwargs is being used now, the proceeding underscore should be removed as it indicates an unused variable.
There was a problem hiding this comment.
Legit embarrassing that I copied and pasted and didn't even think about that.
| @@ -582,6 +588,10 @@ def _zip_package( | |||
| "--no-color", | |||
| ] | |||
|
|
|||
| subprocess_args = {} | |||
| if kwargs.get("python_dontwritebytecode"): | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
ITProKyle
added
feature
pyrsa/pyjwt create scripts with a shebang specific to the build system. Removing them, and setuptools remnants containing various hash values, stops new functions from being built on different systems.
It's possible the
PYTHONDONTWRITEBYTECODEoption is safe to apply unconditionally to all function builds by default, but I'm not 100% certain of the implications of that so I've gated it to an explicit option for now.All of the new options have been intentionally left off the docs for this first pass where we may revise the names.
Discrepancies in the packages looked like:
&