feat: Error on --use-container for dotnet builds #1096
Conversation
There was a problem hiding this comment.
What about integ tests? Can we add --use-container tests to make sure this works. I we could go all out and make sure stdout has the correct logging but we should at least make sure we have integ tests for adding the flag and it doesn't crash or try to do the build in the container.
samcli/lib/build/workflow_config.py
Outdated
| """ | ||
|
|
||
| unsupported = { | ||
| DOTNET_CLIPACKAGE_CONFIG: "We do not support building dotnet Lambda functions within a container. Most " |
There was a problem hiding this comment.
Should this be based on the work workflow or the runtime/lang? If tomorrow we add a second DotNet workflow, do we run the risk of not updating this too?
I don't know the right answer here. My initial thought is we might want to do this on a lang (all DotNet versions and workflows), to keep things consistent but could see a reason to constrain to the Workflow itself.
There was a problem hiding this comment.
Yeah I struggled with this as well. Ultimately, I realized that the workflow implementation is what dictates whether it can run within a container or not. So making the distinction based on workflow config seemed right here. We might find other cases to abstract it differently. Since this is an implementation detail, we can refactor later if needed
sanathkr
changed the title
|
Should we have integration tests for use container with dotnet? |
|
Just pushed integration tests :) |
|
|
||
| with self.assertRaises(ContainerBuildNotSupported) as ctx: | ||
| self.builder._build_function_on_container(config, | ||
| "source_dir", |
There was a problem hiding this comment.
do we need to pass in mode? we may need changes in app_builder.py as well.
There was a problem hiding this comment.
yeah, that's happening as part of the other PR that Jacob created
| @@ -141,7 +141,7 @@ def supports_build_in_container(config): | |||
| """ | |||
|
|
|||
| def _key(c): | |||
| return c.language + c.dependency_manager + str(c.application_framework) | |||
| return str(c.language) + str(c.dependency_manager) + str(c.application_framework) | |||
There was a problem hiding this comment.
Shouldn’t this be the hash function on the object? It’s much cleaner that way.
There was a problem hiding this comment.
The whole point of namedtuple is so you automatically get a sane hash function. I don't want to break that abstraction by defining a custom has that takes only a handful of properties. May be I am wrong here, so let me know if a custom hash is right way of doing things..
Issue #, if available:
Description of changes:
"sam build" for Dotnet does not really need to support building within a container because majority of the dotnet modules don't have native bindings and can compile on any platform and run successfully on Lambda.
PR is marked WIP because the exact error message and UX is still not signed off.
Checklist:
make prpassesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.