Support for gradle builder for Java by sanathkr · Pull Request #1007 · aws/aws-sam-cli

sanathkr · 2019-02-13T16:57:20Z

Issue #, if available:
#880

Description of changes:
This won't build until we release aws-lambda-builders library. But sending the WIP PR to get early feedback.

Checklist:

Write Design Document (Do I need to write a design document?)
Write unit tests
Write/update functional tests
Write/update integration tests
make pr passes
Write documentation

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

jfuss · 2019-02-13T16:59:39Z

requirements/base.txt

 pathlib2~=2.3.2; python_version<"3.4"
 requests==2.20.1
-aws_lambda_builders==0.0.5
+aws_lambda_builders==0.1.0


jfuss · 2019-02-13T17:01:52Z

Don't forget the java8 init templates will need updating from maven to gradle

sanathkr · 2019-02-13T17:09:45Z

Don't forget the java8 init templates will need updating from maven to gradle

It doesn't make sense. We will let the init templates use maven and eventually provide a maven builder. Yes, meanwhile sam init && sam build won't work, but that's a short term problem

sriram-mv · 2019-02-14T22:40:53Z

@sanathkr we need to change https://github.com/awslabs/aws-sam-cli/blob/develop/samcli/lib/build/app_builder.py#L206 to include executable_search_paths

- Also support choosing a gradle workflow by the presence of build.gradle file

jfuss · 2019-02-15T18:18:32Z

samcli/commands/build/command.py

+1. Python 2.7, 3.6, 3.7\n
+4. Nodejs 8.10, 6.10
+4. Ruby 2.5
+5. Java 8


Do we need to call out which Dependency Managers we support?

Hmm, yeah good idea. Will do

jfuss · 2019-02-15T18:19:48Z

samcli/lib/build/app_builder.py

    def _build_function(self, function_name, codeuri, runtime):
-        config = get_workflow_config(runtime)

        # Create the arguments to pass to the builder


This comment is floating. This seems like docstring worthy

sriram-mv · 2019-02-15T18:17:25Z

samcli/lib/build/workflow_config.py

+
+class BasicWorkflowSelector(object):
+    """
+    Basic workflow selector that returns the first available configuration in the given list of configurations


Is this going to be extended in the future? what is the value of this class, if its just returns the first one?

For inheritance.. I didn't create an abstract base class because it didn't seem helpful. But in future we will create a new subclass that will look up Samrc to find the workflow config to use given a runtime. This can be useful for provided runtimes or if customer wants to override a default configuration

samcli/lib/build/workflow_config.py

sriram-mv · 2019-02-15T18:19:29Z

samcli/local/docker/lambda_build_container.py

                      runtime,
                      optimizations,
-                      options):
+                      options,


Just making sure, the protocol version is passed in by importing the builders library right?

sriram-mv · 2019-02-15T18:23:42Z

tests/integration/buildcmd/test_build_cmd.py

+        process = subprocess.Popen(cmdlist, cwd=self.working_dir)
+        process.wait()
+
+        self._verify_built_artifact(self.default_build_dir, self.FUNCTION_LOGICAL_ID,


Can we look at built artifacts verify if we used gradle or gradlew?

Ah good point. I don't know. I will check with Dongie

jfuss · 2019-02-15T18:30:27Z

samcli/lib/build/workflow_config.py

-    try:
-        return workflow_config_by_runtime[runtime]
-    except KeyError:
+    if runtime not in selectors_by_runtime:


It's much cleaner if you do

try: selector = selectors_by_runtime[runtime] config = selector.get_config(code_dir, project_dir) except KeyError: #blah except ValueError: #another blah

Also more pythonic :)

Not really. try/catch blocks should be scoped very tightly to the code you expect to throw. So when you know two statemtents will throw for two different reasons, I would rather have them in separte try blocks or let them throw very specific exceptions - not KeyError & ValueError. That's why I made this way.

jfuss · 2019-02-15T18:32:50Z

samcli/lib/build/workflow_config.py

+
+    @staticmethod
+    def _has_manifest(config, directory):
+        return os.path.exists(os.path.join(directory, config.manifest_name))


One day you will use pathlib.. one day..

when we move to py3. Pathlib in py2 is really confusing

jfuss · 2019-02-15T18:34:40Z

samcli/lib/build/workflow_config.py

+            If none of the supported manifests files are found
+        """
+
+        # Search for manifest first in code directory and then in the project directory.


How this works and is resolved needs to be clearly documented (somewhere)

I will add to the help text for now

sriram-mv · 2019-02-16T00:18:21Z

I dont have any other concerns, will be nice if can determine which executable was used to build in the built artifacts. But I'm not blocking on that. will approve once build passes.

WIP: Support for gradle builder for Java

7863557

jfuss reviewed Feb 13, 2019

View reviewed changes

sanathkr added 3 commits February 14, 2019 08:20

Integration tests for java_gradle

9ba04cf

help text

1b10abd

Merge branch 'develop' into gradle-builder

96d4205

sanathkr changed the title ~~WIP: Support for gradle builder for Java~~ Support for gradle builder for Java Feb 14, 2019

sanathkr added 3 commits February 15, 2019 09:45

Support for adding other executable search paths in Gradle workflow

f0b0207

- Also support choosing a gradle workflow by the presence of build.gradle file

Adding integration test for usinggradlew

8657df1

make pr is happy now

a50688c

jfuss reviewed Feb 15, 2019

View reviewed changes

sriram-mv reviewed Feb 15, 2019

View reviewed changes

jfuss reviewed Feb 15, 2019

View reviewed changes

sanathkr added 3 commits February 15, 2019 14:13

feedback

923b134

py2 fix

96fc50a

Using java8 in travis

673901e

sanathkr added 5 commits February 18, 2019 07:40

travis: Use xenial images to get py37 without sudo

bc4f135

travis: jdk8 is not included by default. apt-get install

e8a67e4

travis: setting java_home

805832f

travis: removing jdk_switcher

3deeb91

travis: adding sudo back

ffc6963

jfuss approved these changes Feb 18, 2019

View reviewed changes

sanathkr merged commit 73a1569 into aws:develop Feb 18, 2019

Conversation

sanathkr commented Feb 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jfuss commented Feb 13, 2019

Uh oh!

sanathkr commented Feb 13, 2019

Uh oh!

sriram-mv commented Feb 14, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sriram-mv commented Feb 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sanathkr commented Feb 13, 2019 •

edited

Loading