fix: validate runtime - #22
Conversation
- Based on the language supplied and lambda runtimes, validate the host runtime to make sure they are compatible.
| from enum import Enum | ||
|
|
||
| from aws_lambda_builders.exceptions import MisMatchRuntimeError | ||
| from aws_lambda_builders.workflows.python_pip.runtime_validator import validate_python_cmd |
There was a problem hiding this comment.
no, you can't do this :( Think of anything inside the workflow folder as a whole separate library that you don't have access to. You can only use the interfaces but not individual implementation.
This allows us to evolve the workflows indendepent of the wrapper library
There was a problem hiding this comment.
Sounds good, I could move it. My motivation behind this was to have language specific validation logic, live inside that specific language's module.
- move validation out of language specific modules
| "artifacts_dir": self.artifacts_dir, | ||
| "scratch_dir": "/ignored", | ||
| "manifest_path": "/ignored", | ||
| "runtime": None, |
There was a problem hiding this comment.
I had to explicitly set this to None, matching it to my python runtime didnt work, and I'm not sure why. Integ tests still work.
There was a problem hiding this comment.
We should set runtime to something like ignored so the validator fails open.
sanathkr
left a comment
sanathkr
left a comment
There was a problem hiding this comment.
Couple of comments about failing open
| :raises ValueError: Unsupported Lambda Runtime | ||
| :raises MisMatchRuntimeError: Version mismatch of the lanugage vs the required runtime | ||
| """ | ||
| try: |
There was a problem hiding this comment.
you could just do a if key in dict instead of try/catch. That's more pythonic
There was a problem hiding this comment.
the reason I didnt want that, was the all if elsing inside that function. maybe a decorator would help? I removed the try catch now.
| except KeyError: | ||
| # The language is not currently supported, so there is no runtime validation. | ||
| return | ||
| if not Runtime.has_value(required_runtime): |
There was a problem hiding this comment.
I would just fail open here. I know this is useful to have, but our runtime checks are never going to be perfect given the long list of languages & dependency managers we have. So I'd rather just fail and log a warning instead.
There was a problem hiding this comment.
Done. warning logged.
| self.builder = LambdaBuilder(language="python", | ||
| dependency_manager="pip", | ||
| application_framework=None) | ||
| self.runtime = "{language}{major}.{minor}".format( |
There was a problem hiding this comment.
nice!! this makes the test work across multiple runtimes :)
| } | ||
|
|
||
|
|
||
| class Runtime(Enum): |
There was a problem hiding this comment.
wait, this needs to work in Py2.7. Enum isn't available there and I am not interested in taking a new dependency on Enum because it doesn't offer a whole lot of functionality in this case ;)
Can you refactor this to remove Enum?
There was a problem hiding this comment.
Sure removed the enum, its a simple class now.
| @@ -0,0 +1,66 @@ | |||
| """ | |||
There was a problem hiding this comment.
Can we also call this validate.py instead of runtime? It is more representative of what this file does.
- add warning logging messages on runtime and language validation.
sanathkr
left a comment
sanathkr
left a comment
There was a problem hiding this comment.
Thanks for making the changes! Looks good
2 participants
host runtime to make sure they are compatible.
Addresses: #11
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.