Skip to content

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

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.

Sign up for GitHub

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

Merged
sanathkr merged 7 commits into from
Apr 8, 2019
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
bd98c09
feat: Bypass containers for dotnet builds
sanathkr Apr 2, 2019
ce2efae
Merge branch 'develop' into dotnet-bypass-container
sanathkr Apr 5, 2019
022a9eb
Fail fast if container build is not supported
sanathkr Apr 5, 2019
a117338
Better error message
sanathkr Apr 5, 2019
376ae11
Integration test for dotnet container build case
sanathkr Apr 5, 2019
50907bb
Deriving a key from workflow config because namedtuples can't be hase…
sanathkr Apr 6, 2019
7b82db8
stringify before creating the key
sanathkr Apr 8, 2019
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
2 changes: 2 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,5 +380,7 @@ $RECYCLE.BIN/

# Temporary scratch directory used by the tests
tests/integration/buildcmd/scratch
tests/integration/testdata/buildcmd/Dotnetcore2.0/bin
tests/integration/testdata/buildcmd/Dotnetcore2.0/obj

# End of https://www.gitignore.io/api/osx,node,macos,linux,python,windows,pycharm,intellij,sublimetext,visualstudiocode
15 changes: 9 additions & 6 deletions samcli/commands/build/command.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@
from samcli.commands._utils.options import template_option_without_build, docker_common_options, \
parameter_override_option
from samcli.commands.build.build_context import BuildContext
from samcli.lib.build.app_builder import ApplicationBuilder, BuildError, UnsupportedBuilderLibraryVersionError
from samcli.lib.build.app_builder import ApplicationBuilder, BuildError, UnsupportedBuilderLibraryVersionError, \
ContainerBuildNotSupported
from samcli.lib.build.workflow_config import UnsupportedRuntimeException
from samcli.commands._utils.template import move_template

Expand All @@ -31,9 +32,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 (without --use-container flag)\n
\b
Examples
--------
Expand Down Expand Up @@ -147,8 +149,9 @@ def do_cli(template, # pylint: disable=too-many-locals

click.secho(msg, fg="yellow")

except (UnsupportedRuntimeException, BuildError, UnsupportedBuilderLibraryVersionError) as ex:
click.secho("Build Failed", fg="red")
except (UnsupportedRuntimeException, BuildError, UnsupportedBuilderLibraryVersionError,
ContainerBuildNotSupported) as ex:
click.secho("\nBuild Failed", fg="red")
raise UserException(str(ex))


Expand Down
10 changes: 9 additions & 1 deletion samcli/lib/build/app_builder.py
View file
Open in desktop
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 All @@ -33,6 +33,10 @@ def __init__(self, container_name, error_msg):
Exception.__init__(self, msg.format(container_name=container_name, error_msg=error_msg))


class ContainerBuildNotSupported(Exception):
pass


class BuildError(Exception):
pass

Expand Down Expand Up @@ -224,6 +228,10 @@ def _build_function_on_container(self, # pylint: disable=too-many-locals
if not self._container_manager.is_docker_reachable:
raise BuildError("Docker is unreachable. Docker needs to be running to build inside a container.")

container_build_supported, reason = supports_build_in_container(config)
if not container_build_supported:
raise ContainerBuildNotSupported(reason)

# If we are printing debug logs in SAM CLI, the builder library should also print debug logs
log_level = LOG.getEffectiveLevel()

Expand Down
35 changes: 35 additions & 0 deletions samcli/lib/build/workflow_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,41 @@ def get_workflow_config(runtime, code_dir, project_dir):
.format(runtime, str(ex)))


def supports_build_in_container(config):
"""
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.
"""

def _key(c):
return str(c.language) + str(c.dependency_manager) + str(c.application_framework)
Copy link
Contributor

@jfuss jfuss Apr 8, 2019

Choose a reason for hiding this comment

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

Shouldn’t this be the hash function on the object? It’s much cleaner that way.

Copy link
Contributor Author

@sanathkr sanathkr Apr 8, 2019

Choose a reason for hiding this comment

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

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..


# This information could have beeen bundled inside the Workflow Config object. But we this way because
# ultimately the workflow's implementation dictates whether it can run within a container or not.
# A "workflow config" is like a primary key to identify the workflow. So we use the config as a key in the
# map to identify which workflows can support building within a container.

unsupported = {
_key(DOTNET_CLIPACKAGE_CONFIG): "We do not support building .NET Core Lambda functions within a container. "
"Try building without the container. Most .NET Core functions will build "
"successfully.",
}

if _key(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
18 changes: 18 additions & 0 deletions tests/integration/buildcmd/test_build_cmd.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,6 +356,24 @@ def test_with_dotnetcore(self, runtime, code_uri, use_container):

self.verify_docker_container_cleanedup(runtime)

@parameterized.expand([
("dotnetcore2.0", "Dotnetcore2.0"),
("dotnetcore2.1", "Dotnetcore2.1"),
])
def test_must_fail_with_container(self, runtime, code_uri):
use_container = True
overrides = {"Runtime": runtime, "CodeUri": code_uri,
"Handler": "HelloWorld::HelloWorld.Function::FunctionHandler"}
cmdlist = self.get_command_list(use_container=use_container,
parameter_overrides=overrides)

LOG.info("Running Command: {}".format(cmdlist))
process = subprocess.Popen(cmdlist, cwd=self.working_dir)
process.wait()

# Must error out, because container builds are not supported
self.assertEquals(process.returncode, 1)

def _verify_built_artifact(self, build_dir, function_logical_id, expected_files):

self.assertTrue(build_dir.exists(), "Build directory should be created")
Expand Down
19 changes: 18 additions & 1 deletion tests/unit/lib/build_module/test_app_builder.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

from samcli.lib.build.app_builder import ApplicationBuilder,\
UnsupportedBuilderLibraryVersionError, BuildError, \
LambdaBuilderError
LambdaBuilderError, ContainerBuildNotSupported


class TestApplicationBuilder_build(TestCase):
Expand Down Expand Up @@ -325,6 +325,23 @@ def test_must_raise_on_docker_not_running(self):
self.assertEquals(str(ctx.exception),
"Docker is unreachable. Docker needs to be running to build inside a container.")

@patch("samcli.lib.build.app_builder.supports_build_in_container")
def test_must_raise_on_unsupported_container_build(self, supports_build_in_container_mock):
config = Mock()

reason = "my reason"
supports_build_in_container_mock.return_value = (False, reason)

with self.assertRaises(ContainerBuildNotSupported) as ctx:
self.builder._build_function_on_container(config,
"source_dir",
Copy link
Contributor

@sriram-mv sriram-mv Apr 8, 2019

Choose a reason for hiding this comment

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

do we need to pass in mode? we may need changes in app_builder.py as well.

Copy link
Contributor Author

@sanathkr sanathkr Apr 8, 2019

Choose a reason for hiding this comment

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

yeah, that's happening as part of the other PR that Jacob created

"artifacts_dir",
"scratch_dir",
"manifest_path",
"runtime")

self.assertEquals(str(ctx.exception), reason)


class TestApplicationBuilder_parse_builder_response(TestCase):

Expand Down