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

feat: Error on --use-container for dotnet builds #1096

Merged
merged 7 commits into from
Apr 8, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions samcli/commands/build/command.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,10 @@
Supported Runtimes
------------------
1. Python 2.7, 3.6, 3.7 using PIP\n
4. Nodejs 8.10, 6.10 using NPM
4. Ruby 2.5 using Bundler
5. Java 8 using Gradle
2. Nodejs 8.10, 6.10 using NPM\n
3. Ruby 2.5 using Bundler\n
4. Java 8 using Gradle\n
5. Dotnetcore2.0 and 2.1 using Dotnet CLI\n
\b
Examples
--------
Expand Down
10 changes: 8 additions & 2 deletions samcli/lib/build/app_builder.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
from aws_lambda_builders.builder import LambdaBuilder
from aws_lambda_builders.exceptions import LambdaBuilderError
from aws_lambda_builders import RPC_PROTOCOL_VERSION as lambda_builders_protocol_version
from .workflow_config import get_workflow_config
from .workflow_config import get_workflow_config, supports_build_in_container


LOG = logging.getLogger(__name__)
Expand Down Expand Up @@ -180,7 +180,13 @@ def _build_function(self, function_name, codeuri, runtime):
# By default prefer to build in-process for speed
build_method = self._build_function_in_process
if self._container_manager:
build_method = self._build_function_on_container

container_build_supported, reason = supports_build_in_container(config)
if container_build_supported:
build_method = self._build_function_on_container
else:
LOG.warning(reason)
sanathkr marked this conversation as resolved.
Show resolved Hide resolved
LOG.warning("Continuing build without a container")

return build_method(config,
code_dir,
Expand Down
26 changes: 26 additions & 0 deletions samcli/lib/build/workflow_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,32 @@ def get_workflow_config(runtime, code_dir, project_dir):
.format(runtime, str(ex)))


def supports_build_in_container(config):
sanathkr marked this conversation as resolved.
Show resolved Hide resolved
"""
Given a workflow config, this method provides a boolean on whether the workflow can run within a container or not.

Parameters
----------
config namedtuple(Capability)
Config specifying the particular build workflow

Returns
-------
tuple(bool, str)
True, if this workflow can be built inside a container. False, along with a reason message if it cannot be.
"""

unsupported = {
DOTNET_CLIPACKAGE_CONFIG: "We do not support building dotnet Lambda functions within a container. Most "
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

"dotnet functions can be successfully built without a container.",
}

if config in unsupported:
return False, unsupported[config]

return True, None


class BasicWorkflowSelector(object):
"""
Basic workflow selector that returns the first available configuration in the given list of configurations
Expand Down
41 changes: 41 additions & 0 deletions tests/unit/lib/build_module/test_app_builder.py
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,47 @@ def test_must_build_in_container(self, osutils_mock, get_workflow_config_mock):
manifest_path,
runtime)

@patch("samcli.lib.build.app_builder.supports_build_in_container")
@patch("samcli.lib.build.app_builder.get_workflow_config")
@patch("samcli.lib.build.app_builder.osutils")
def test_must_fallback_to_build_in_process_for_unsupported_containers(self,
osutils_mock,
get_workflow_config_mock,
supports_build_in_container_mock):
function_name = "function_name"
codeuri = "path/to/source"
runtime = "runtime"
scratch_dir = "scratch"
config_mock = get_workflow_config_mock.return_value = Mock()
config_mock.manifest_name = "manifest_name"

osutils_mock.mkdir_temp.return_value.__enter__ = Mock(return_value=scratch_dir)
osutils_mock.mkdir_temp.return_value.__exit__ = Mock()

self.builder._build_function_on_container = Mock()
self.builder._build_function_in_process = Mock()

code_dir = "/base/dir/path/to/source"
artifacts_dir = "/build/dir/function_name"
manifest_path = os.path.join(code_dir, config_mock.manifest_name)

# Setting the container manager will make us use the container
self.builder._container_manager = Mock()

# Mark the container build as unsupported
supports_build_in_container_mock.return_value = (False, "reason")

self.builder._build_function(function_name, codeuri, runtime)

self.builder._build_function_on_container.assert_not_called()
self.builder._build_function_in_process.assert_called_with(config_mock,
code_dir,
artifacts_dir,
scratch_dir,
manifest_path,
runtime)
supports_build_in_container_mock.assert_called_with(config_mock)


class TestApplicationBuilder_build_function_in_process(TestCase):

Expand Down