Update task sample for java 11 #1382

averikitsch · 2019-04-12T15:42:15Z

No description provided.

kurtisvg · 2019-04-17T01:20:08Z

appengine-java11/pom.xml

@@ -0,0 +1,49 @@
+<!--


This pom doesn't seem to do anything - it should be able to be removed.

This still needs to be removed.

kurtisvg · 2019-04-17T01:23:01Z

appengine-java11/tasks/README.md

+First, your project ID:
+
+```
+export GOOGLE_CLOUD_PROJECT=<YOUR_GOOGLE_CLOUD_PROJECT>


Best practice to escape variable contents with quotes in bash.

Suggested change

export GOOGLE_CLOUD_PROJECT=<YOUR_GOOGLE_CLOUD_PROJECT>

export GOOGLE_CLOUD_PROJECT="<YOUR_GOOGLE_CLOUD_PROJECT>"

kurtisvg · 2019-04-17T01:23:24Z

appengine-java11/tasks/README.md

+created can be listed with `gcloud beta tasks queues list`.
+
+```
+export QUEUE_ID=my-appengine-queue


Suggested change

export QUEUE_ID=my-appengine-queue

export QUEUE_ID="my-appengine-queue"

kurtisvg · 2019-04-17T01:23:34Z

appengine-java11/tasks/README.md

+location is "us-central1").
+
+```
+export LOCATION_ID=<YOUR_ZONE>


Suggested change

export LOCATION_ID=<YOUR_ZONE>

export LOCATION_ID="<YOUR_ZONE>"

kurtisvg · 2019-04-17T01:24:04Z

appengine-java11/tasks/pom.xml

+    The parent pom defines common style checks and testing strategies for our samples.
+    Removing or replacing it should not affect the execution of the samples in anyway.
+  -->
+  <!-- <parent>


Reminder to uncomment this before submitting.

Still looks commented to me.

kurtisvg · 2019-04-17T01:25:00Z

appengine-java11/tasks/pom.xml

+
+  <dependencies>
+    <dependency>
+      <groupId>com.google.appengine.demo</groupId>


It might be good to have a comment or link to instructions on where the user can go to build this requirement, since it's unable to be pulled down from maven central.

A relative path might also work? Up to you.

If the user comes to this sample from a link, it's unclear where they have to go for this (they may not realize the entire structure of the samples). Please expand either stating "from this repository" or add a full link so the user can take a look.

kurtisvg · 2019-04-17T01:31:00Z

appengine-java11/tasks/src/main/java/com/example/task/CreateTask.java

+import java.time.Clock;
+import java.time.Instant;
+
+import org.apache.commons.cli.CommandLine;


There has been a bunch of pushback on adding CLI's to samples (the new sample format mandates you don't unless it's an explicit requirement for the sample). This is because a) it ends up being a bunch of boilerplate code, b) everyone uses a different cli framework and so it becomes a pain to maintain or update future versions. (Java is also particularly painful since it's hard to remember the correct execution syntax.)

A better alternative would be to add an HTTP endpoint that covers the task creation instead, and provide the user with instructions using curl.

(This is up to you since there is no collective agreement on when the new samples format becomes mandatory)

Removed the larger CLI; however still using main in order to be executable.

kurtisvg · 2019-04-17T01:33:22Z

appengine-java11/tasks/src/main/java/com/example/task/CreateTask.java

+      .longOpt("project-id")
+      .desc("The Google Cloud Project, if not set as GOOGLE_CLOUD_PROJECT env var.")
+      .hasArg()
+      .argName("project-id")


I would suggest something like the following to make it clear that the user needs to provide their own data:

Suggested change

.argName("project-id")

.argName("your-project-id")

kurtisvg · 2019-04-17T01:34:57Z

appengine-java11/tasks/src/main/java/com/example/task/CreateTask.java

+
+    // Instantiates a client.
+    try (CloudTasksClient client = CloudTasksClient.create()) {
+


Nix the newline

kurtisvg · 2019-04-17T01:37:57Z

appengine-java11/tasks/src/main/java/com/example/task/CreateTask.java

+      // Construct the task body.
+      Task.Builder taskBuilder = Task
+          .newBuilder()
+          .setAppEngineHttpRequest(AppEngineHttpRequest.newBuilder()


This line break looks like it might be a style violation (checkstyle should catch it when you get it added)

Still seems wrong - see "Where to Line Break"

kurtisvg · 2019-04-17T21:22:55Z

appengine-java11/tasks/pom.xml

+    The parent pom defines common style checks and testing strategies for our samples.
+    Removing or replacing it should not affect the execution of the samples in anyway.
+  -->
+  <!-- <parent>


Still looks commented to me.

kurtisvg · 2019-04-17T21:23:21Z

appengine-java11/pom.xml

@@ -0,0 +1,49 @@
+<!--


This still needs to be removed.

kurtisvg · 2019-04-17T21:27:50Z

appengine-java11/tasks/pom.xml

+
+  <dependencies>
+    <dependency>
+      <groupId>com.google.appengine.demo</groupId>


If the user comes to this sample from a link, it's unclear where they have to go for this (they may not realize the entire structure of the samples). Please expand either stating "from this repository" or add a full link so the user can take a look.

kurtisvg · 2019-04-17T21:28:24Z

appengine-java11/tasks/src/main/java/com/example/task/CreateTask.java

+      // Construct the task body.
+      Task.Builder taskBuilder = Task
+          .newBuilder()
+          .setAppEngineHttpRequest(AppEngineHttpRequest.newBuilder()


Still seems wrong - see "Where to Line Break"

…/java-docs-samples into tasks-sample

averikitsch · 2019-04-23T15:43:00Z

@kurtisvg I updated the task handler to a Spring Boot app and moved it to a separate directory.

kurtisvg · 2019-04-23T15:52:17Z

appengine-java11/tasks-handler/pom.xml

+	<artifactId>task-handler-j11</artifactId>
+	<version>0.0.1-SNAPSHOT</version>
+
+	<parent>


Can you run a formatter on this POM? IntelliJ has on built in

kurtisvg · 2019-04-23T15:53:31Z

appengine-java11/tasks-handler/pom.xml

+
+	<parent>
+    <groupId>org.springframework.boot</groupId>
+    <artifactId>spring-boot-starter-parent</artifactId>


This isn't the shared config parent - is that intentional?

This is needed for Spring Boot. Adding the dependencies individually was making it more complicated but I can do that instead.

I think this should be fine if the shared-config work as a dependency instead

Figured out how cleanly remove this.

kurtisvg · 2019-04-23T15:53:55Z

appengine-java11/tasks-handler/pom.xml

+    <groupId>org.springframework.boot</groupId>
+    <artifactId>spring-boot-starter-parent</artifactId>
+    <version>2.1.4.RELEASE</version>
+    <relativePath/> <!-- lookup parent from repository -->


Is this supposed to be an open/close tag?

kurtisvg · 2019-04-23T15:55:35Z

appengine-java11/tasks-handler/mvnw

@@ -0,0 +1,286 @@
+#!/bin/sh
+# ----------------------------------------------------------------------------


Do we need to include a maven wrapper with this sample? The licensing may have ramifications.

kurtisvg · 2019-04-23T15:56:42Z

appengine-java11/tasks-handler/pom.xml

+      <dependency>
+        <groupId>org.springframework.cloud</groupId>
+        <artifactId>spring-cloud-dependencies</artifactId>
+        <version>${spring-cloud.version}</version>


Does this need to be a property? DPE bot is unable to update versions that use these.

kurtisvg · 2019-04-23T15:57:51Z

appengine-java11/tasks-handler/src/main/java/com/example/appengine/taskhandler/Application.java

@@ -0,0 +1,34 @@
+/*
+ * Copyright 2017-2019 the original author or authors.


This license header is incorrect.

kurtisvg · 2019-04-23T15:58:33Z

...a11/tasks-handler/src/main/java/com/example/appengine/taskhandler/TaskHandlerController.java

@@ -0,0 +1,41 @@
+/*
+ * Copyright 2017-2019 the original author or authors.


This license header is incorrect.

appengine-java11/tasks/pom.xml

kurtisvg · 2019-04-23T17:35:55Z

appengine-java11/tasks-handler/pom.xml

+    <!--
+    The parent pom defines common style checks and testing strategies for our samples.
+    Removing or replacing it should not affect the execution of the samples in anyway.
+  -->


nit: Indent this.

kurtisvg · 2019-04-23T17:36:04Z

appengine-java11/tasks-handler/pom.xml

+        <groupId>com.google.cloud.samples</groupId>
+        <artifactId>shared-configuration</artifactId>
+        <version>1.0.11</version>
+        <relativePath></relativePath>


You don't need to specify a relative path - should pull from maven.

appengine-java11/tasks-handler/pom.xml

appengine-java11/tasks-handler/src/main/appengine/app.yaml

averikitsch added 2 commits April 12, 2019 08:38

update task sample for java 11

b886ae7

remove region tags

ca4186e

averikitsch requested a review from a team April 12, 2019 15:42

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 12, 2019

averikitsch added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 12, 2019

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 12, 2019

averikitsch requested a review from lesv April 16, 2019 23:58

kurtisvg suggested changes Apr 17, 2019

View reviewed changes

kurtisvg removed the request for review from lesv April 17, 2019 01:42

Remove CLI and update readme

4f49b66

kurtisvg suggested changes Apr 17, 2019

View reviewed changes

averikitsch added 11 commits April 17, 2019 18:04

respond to PR comments

b0a39ee

Update pom.xml

8bc5511

Update scopes

62db987

Merge branch 'master' into tasks-sample

36119a4

Remove systempath dependency

063c58f

Merge branch 'tasks-sample' of https://github.com/GoogleCloudPlatform…

abca31d

…/java-docs-samples into tasks-sample

Update tasks sample to use spring boot handler

e3e82f3

Merge branch 'master' into tasks-sample

3f3a60f

Don't fail on missing web.xml

88a5bad

Merge branch 'tasks-sample' of https://github.com/GoogleCloudPlatform…

ad54674

…/java-docs-samples into tasks-sample

Add configuration for missing web.xml

8cbffeb

kurtisvg suggested changes Apr 23, 2019

View reviewed changes

averikitsch added 3 commits April 23, 2019 10:01

Update licenses and styling

b9f0191

checkstyle

eef00ac

Format pom

ff04f6f

kurtisvg suggested changes Apr 23, 2019

View reviewed changes

Update style

1429e23

kurtisvg approved these changes Apr 23, 2019

View reviewed changes

averikitsch merged commit f7ab04f into master Apr 23, 2019

averikitsch deleted the tasks-sample branch April 23, 2019 18:58

	export GOOGLE_CLOUD_PROJECT=<YOUR_GOOGLE_CLOUD_PROJECT>
	export GOOGLE_CLOUD_PROJECT="<YOUR_GOOGLE_CLOUD_PROJECT>"

	export QUEUE_ID=my-appengine-queue
	export QUEUE_ID="my-appengine-queue"

	export LOCATION_ID=<YOUR_ZONE>
	export LOCATION_ID="<YOUR_ZONE>"


		// Instantiates a client.
		try (CloudTasksClient client = CloudTasksClient.create()) {

		@@ -0,0 +1,286 @@
		#!/bin/sh
		# ----------------------------------------------------------------------------

		@@ -0,0 +1,34 @@
		/*
		* Copyright 2017-2019 the original author or authors.

		@@ -0,0 +1,41 @@
		/*
		* Copyright 2017-2019 the original author or authors.

Update task sample for java 11 #1382

Update task sample for java 11 #1382

Uh oh!

Conversation

averikitsch commented Apr 12, 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!

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!

kurtisvg Apr 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

averikitsch commented Apr 23, 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!

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!

kurtisvg Apr 17, 2019 •

edited

Loading