Creating `aspect` option (ALL, CODE, PACKAGE) #475

garrettjonesgoogle · 2018-07-25T15:58:39Z

This will only work for C# so far; after googleapis/gapic-generator#2166 , it will work for Java too.

vam-google

In general looks good, with several comments.

artman/pipelines/grpc_generation.py


    def get_tasks(self, **kwargs):
        tasks = self.get_grpc_codegen_tasks(**kwargs)
        tasks += self._get_publish_tasks(**kwargs)
        return task_utils.instantiate_tasks(tasks, kwargs)

    def get_grpc_codegen_tasks(self, **kwargs):
+        if self.language == 'java':


artman/cli/main.py

@@ -179,6 +179,11 @@ def parse_args(*args):
        type=str,
        help='[Required] Name of the artifact for artman to generate. Must '
        'match an artifact in the artman config yaml.')
+    parser_generate.add_argument(
+        '--aspect',


artman/tasks/gapic_tasks.py

@@ -116,8 +117,19 @@ def execute(self, language, toolkit_path, descriptor_set, service_yaml,
        if service_yaml:
            args = args + ['--service_yaml=' + os.path.abspath(service_yaml)]
        args = args + gapic_args
+
+        gapic_artifact = ''
+        if aspect == 'ALL':


artman/config/loader.py

@@ -54,7 +54,7 @@
 }


-def load_artifact_config(artman_config_path, artifact_name):
+def load_artifact_config(artman_config_path, artifact_name, aspect=None):


vam-google

LGTM, but I still don't like the huge get_grpc_codegen_tasks method.

artman/cli/main.py

@@ -179,6 +179,11 @@ def parse_args(*args):
        type=str,
        help='[Required] Name of the artifact for artman to generate. Must '
        'match an artifact in the artman config yaml.')
+    parser_generate.add_argument(
+        '--aspect',


artman/pipelines/grpc_generation.py


    def get_tasks(self, **kwargs):
        tasks = self.get_grpc_codegen_tasks(**kwargs)
        tasks += self._get_publish_tasks(**kwargs)
        return task_utils.instantiate_tasks(tasks, kwargs)

    def get_grpc_codegen_tasks(self, **kwargs):
+        if self.language == 'java':


garrettjonesgoogle · 2018-07-25T23:10:59Z

ALL vs CODE_AND_PACKAGE: I considered the second, but I wanted to allow for adding a third item, and the default should be to include everything I think.

garrettjonesgoogle · 2018-07-25T23:22:57Z

I split apart the long function, PTAL

Creating aspect option (ALL, CODE, PACKAGE)

eb714f7

garrettjonesgoogle requested a review from vam-google July 25, 2018 15:58

garrettjonesgoogle mentioned this pull request Jul 25, 2018

Using --aspect CODE from artman; supporting java_proto googleapis/google-cloud-java#3507

Merged

vam-google reviewed Jul 25, 2018

View reviewed changes

vam-google approved these changes Jul 25, 2018

View reviewed changes

PR feedback

f8fa838

garrettjonesgoogle merged commit 8662dfe into googleapis:master Jul 26, 2018

alexander-fenster mentioned this pull request Jul 26, 2018

smoke tests are failing #479

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Creating `aspect` option (ALL, CODE, PACKAGE) #475

Creating `aspect` option (ALL, CODE, PACKAGE) #475

garrettjonesgoogle commented Jul 25, 2018

vam-google left a comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

vam-google left a comment

This comment was marked as spam.

This comment was marked as spam.

garrettjonesgoogle commented Jul 25, 2018

garrettjonesgoogle commented Jul 25, 2018

Creating aspect option (ALL, CODE, PACKAGE) #475

Creating aspect option (ALL, CODE, PACKAGE) #475

Conversation

garrettjonesgoogle commented Jul 25, 2018

vam-google left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

vam-google left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

garrettjonesgoogle commented Jul 25, 2018

garrettjonesgoogle commented Jul 25, 2018

Creating `aspect` option (ALL, CODE, PACKAGE) #475

Creating `aspect` option (ALL, CODE, PACKAGE) #475