Initial ChatBot implementation #1

yuri-sergiichuk · 2020-06-09T13:48:49Z

The PR is here to address SpineEventEngine/config#403 issue.
In the PR I have drafted the initial implementation of the ChatBot applications that allows notifying Spine developers about Travis CI builds failures through the Google Chat.

The idea of the app is to use Travis CI API in order to fetch the latest master branch build status for the registered repositories and notify Spine Developers to a particular Google Chat room and thread with the build state.

yuri-sergiichuk · 2020-06-09T15:34:49Z

@armiol PTAL.

armiol

@yuri-sergiichuk please refer to the latest Spine build scripts and modify this project accordingly:

Kotlin in Gradle scripts instead of Groovy;
application version defined in a separate file;
(perhaps, in future PRs) static code analysis is configured: Error Prone, PMD, Checkstyle;
(consult with @dmdashenkov) there is already Gradle 6.5, but Dmitry mentioned there was an issue in their release which breaks some of our Spine plugin code.

In addition to the project setup:

Public API must be documented.
Avoid the wildcard imports. Use the latest inspection profile from Spine. Also, it helps to use the most recent IDEA (I am using the latest EAP at the moment) — it detects more warnings and errors, some of them even inline as you type.
Please have a README.md stating the name of the application AND some details. So if you wish to speak of Micronaut, do that in the document body, not in the title. Also, choosing Java 11 needs some explanation in terms of the target deployment environment — I don't know the plan, so I am not able to comment on this matter.
(optional) I am not sure about Log4J. To me, this is a very early decision.

armiol

@yuri-sergiichuk I am 33% done. Please see my comments I left so far.

.gitignore

README.md

armiol · 2020-06-17T18:55:35Z

README.md

+```
+
+The application will be available at `127.0.0.1:${LOCAL_PORT}` (e.g. `127.0.0.1:9090`). 
+Locally-supplied GCP credentials are mount into the image directly.


https://simple.wiktionary.org/wiki/mount

armiol · 2020-06-17T18:57:59Z

README.md

+The application will be available at `127.0.0.1:${LOCAL_PORT}` (e.g. `127.0.0.1:9090`). 
+Locally-supplied GCP credentials are mount into the image directly.
+
+For detailed ADC credentials guide for Docker see example Cloud Run [guide][cloud-run-local-guide].


I have no idea what an ADC is. I would put it in its full form first followed by a short form when mentioning for the first time. E.g. "Black-box bounded context (BBBC) is ...". Otherwise, you risk losing a reader who is going to leave for googling at this point.

armiol · 2020-06-17T18:59:21Z

buildSrc/src/main/kotlin/java-convention.gradle.kts

+    // Explicitly states the encoding of the source and test source files, ensuring
+    // correct execution of the `javac` task.
+    options.encoding = "UTF-8"
+    options.compilerArgs.addAll(listOf("-Xlint:unchecked", "-Xlint:deprecation"))


BTW, we are migrating Spine to the compiler configuration in which we fail if there is at least one warning. So you may add -Werror here as well.

armiol · 2020-06-17T19:14:32Z

google-chat-bot/src/main/java/io/spine/chatbot/InitController.java

+                });
+    }
+
+    private static RegisterRepository newRegisterRepoCommand(Repository repository,


See my comment to a similar use case above.

armiol · 2020-06-17T19:15:22Z

google-chat-bot/src/main/java/io/spine/chatbot/InitController.java

+        _info().log("Performing initial application state initialization.");
+        var client = ChatBotClient.inProcessClient(Application.SERVER_NAME);
+        var spineOrgId = newOrganizationId("SpineEventEngine");
+        registerOrganization(client, spineOrgId, spaceName);


Do we validate spaceName? Consider having a Protobuf message which would travel from this point further.

armiol · 2020-06-17T19:15:35Z

google-chat-bot/src/main/java/io/spine/chatbot/InitController.java

+     */
+    @Get
+    public String initWatchedResources(@QueryValue String spaceName) {
+        _info().log("Performing initial application state initialization.");


I would log the query value as well.

armiol · 2020-06-17T19:16:11Z

google-chat-bot/src/main/java/io/spine/chatbot/InitController.java

+        return "Successfully initialized";
+    }
+
+    private static void registerWatchedRepos(ChatBotClient client, OrganizationId spineOrgId) {


Can we please not have any operations in controllers other than taking and validating the parameters and sending out responses?

armiol · 2020-06-17T19:16:52Z

google-chat-bot/src/main/java/io/spine/chatbot/api/BuildStateUpdates.java

+    /**
+     * Creates a new {@link BuildState} update message of of the supplied state and the thread name.
+     *
+     * <p>If the thread name is empty, assumes that the update message should be sent


And what if it's worse than empty? What it is null?

yuri-sergiichuk · 2020-06-19T16:54:39Z

@armiol, PTAL again.

armiol

@yuri-sergiichuk 75% reviewed. See my comments.

armiol · 2020-06-20T10:25:59Z

buildSrc/src/main/kotlin/deps.kt

+    const val junit5 = "5.6.2"
+    const val truth = "1.0.1"
+    const val micronaut = "2.0.0.RC1"
+    const val spineGcloud = "1.5.0"


Please review the Spine versions declared in different files. I.e. the gcloud-java version is here, and bootstrap version is in build.gradle.kts.

That's a limitation of the Kotlin build scripts support that we're facing right now.

In order to be able to utilize Kotlin DSL pre-compiled plugins and configure build logic in e.g. java-convention.gradle.kts, the plugins we're using should be added as dependencies to build.gradle.kts. And we're not able to import and use deps.kt there, cause we're just going to compile it.
It's a chicken and an egg problem here.

The workaround here maybe be to fall back to awkward buildScript configurations and multiple apply from constructs.

I created a separate issue for this: #3.

armiol · 2020-06-20T10:27:08Z

cloudbuild.yaml

@@ -0,0 +1,24 @@
+# TODO:2020-06-17:yuri-sergiichuk: the build fails in the cloud with


Please review. The rule is either to address the todo item, or have an issue in the repository linked here.

armiol · 2020-06-20T10:28:56Z

google-chat-bot/src/main/java/io/spine/chatbot/IncomingEventsController.java

+        var chatEventJson = decodeBase64Json(message.getData());
+        _debug().log("Received a new chat event: %s", chatEventJson);
+        ChatEvent chatEvent = Json.fromJson(chatEventJson, ChatEvent.class);
+        var client = ChatBotClient.inProcessClient(Application.SERVER_NAME);


Starting this line, the code does not belong to a controller.

armiol · 2020-06-20T10:31:34Z

google-chat-bot/src/main/java/io/spine/chatbot/RepositoriesController.java

+                .command(checkRepositoryBuild)
+                .onStreamingError(RepositoriesController::throwProcessingError)
+                .post();
+        subscriptions.forEach(botClient::cancelSubscription);


Not sure I understand this trick with canceling the subscriptions. Please document this method.

That's done accordingly to the @apiNote in the post method:

@apiNote Subscriptions should be cancelled to free up client and server resources * required for their maintenance. It is not possible to cancel the returned * subscription in an automatic way because of the following. * Subscriptions by nature are asynchronous and infinite requests. * Even that we know expected types of the events produced by the command, only the * client code "knows" how many of them it expects. Also, some events may not arrive * because of communication or business logic reasons. That's why the returned * subscriptions should be cancelled by the client code when it no longer needs it.

armiol · 2020-06-20T10:33:59Z

google-chat-bot/src/main/java/io/spine/chatbot/api/BuildStateUpdates.java

+     * Creates a new {@link BuildState} update message of of the supplied state and the thread
+     * name.
+     *
+     * <p>If the thread name is {@code null} or empty, assumes that the update message should be


Let's settle down on what means what. I would say that if the thread name was passed as null, then a new thread should have been engaged. The empty string value should be treated as an input mistake.

armiol · 2020-06-20T11:10:09Z

google-chat-bot/src/main/proto/spine/chatbot/github/repository_commands.proto

+
+    // ID of the organization the repository is related to if any.
+    OrganizationId organization = 5;
+}


google-chat-bot/src/main/proto/spine/chatbot/google/chat/incoming/incoming_chat_messages.proto

armiol · 2020-06-20T11:11:54Z

google-chat-bot/src/main/proto/spine/chatbot/google/chat/space.proto

+
+    // The display name (only if the space is a room).
+    string display_name = 5;
+}


google-chat-bot/src/main/proto/spine/chatbot/google/chat/space_commands.proto

yuri-sergiichuk · 2020-06-21T14:40:01Z

@armiol, PTAL again.

Trigger builds as recovered only if we previously had them as failed in the process.

… itself. E.g. grab Repository and branch name from the response

alexander-yevsyukov

Please see my comments.

.idea/codeStyleSettings.xml

google-chat-bot/src/main/java/io/spine/chatbot/ChatBotServerEnvironment.java

google-chat-bot/src/main/java/io/spine/chatbot/Application.java

google-chat-bot/src/main/java/io/spine/chatbot/api/google/chat/BuildStateUpdates.java

google-chat-bot/src/main/java/io/spine/chatbot/api/google/chat/GoogleChat.java

google-chat-bot/src/main/proto/spine/chatbot/travis/travis.proto

…nt and server code

yuri-sergiichuk · 2020-07-15T15:09:10Z

@alexander-yevsyukov @armiol PTAL again.

google-chat-bot/src/test/java/io/spine/chatbot/CanFailFast.java

alexander-yevsyukov

LGTM, with some comments and suggestions

alexander-yevsyukov · 2020-07-15T16:55:16Z

google-chat-bot/src/main/java/io/spine/chatbot/google/chat/GoogleChat.java

+    private final HangoutsChat chat;
+
+    GoogleChat(HangoutsChat chat) {
+        this.chat = chat;


Please check for null input.

alexander-yevsyukov · 2020-07-15T17:03:47Z

google-chat-bot/src/main/proto/google/pubsub/v1/pubsub_push.proto

@@ -0,0 +1,25 @@
+syntax = "proto3";
+
+package google.pubsub.v1;


This needs fixing. We cannot pretend we're Google.

alexander-yevsyukov · 2020-07-15T17:31:03Z

google-chat-bot/src/main/java/io/spine/chatbot/github/repository/build/BuildStateMixin.java

+
+    /**
+     * Creates an instance of the {@linkplain Build.State build state} of out its
+     * string representation.


Suggested change

* string representation.

* string representation, ignoring the case.

alexander-yevsyukov · 2020-07-15T17:33:54Z

google-chat-bot/src/main/java/io/spine/chatbot/github/repository/build/BuildStateMixin.java

+                return buildState;
+            }
+        }
+        throw newIllegalArgumentException(


If you're sure that Build.State is named according to Java conventions, you can probably do this:

return Enum.valueOf(Build.State.class, state.toUpperCase());

It would throw IllegalArgumentException if there's no a member with the name.

alexander-yevsyukov · 2020-07-15T17:38:54Z

google-chat-bot/src/main/java/io/spine/chatbot/IncomingEventsController.java

+        var message = pushNotification.getMessage();
+        var chatEventJson = message.getData()
+                                   .toStringUtf8();
+        _debug().log("Received a new chat event: %s", chatEventJson);


We finish most of the log messages with periods because they are sentences. If you expect JSON to be long, how about having it starting from a new line then?

alexander-yevsyukov · 2020-07-15T17:40:15Z

google-chat-bot/src/main/java/io/spine/chatbot/delivery/DistributedDelivery.java

+final class DistributedDelivery {
+
+    /** The number of shards used for the signal delivery. **/
+    private static final int NUMBER_OF_SHARDS = 50;


@armiol, do we need that many for this kind of app?

@alexander-yevsyukov @yuri-sergiichuk That seems to be a proper amount.

alexander-yevsyukov · 2020-07-15T17:50:03Z

google-chat-bot/src/main/java/io/spine/chatbot/github/Slugs.java

+    /**
+     * Creates a new {@code Slug} with the specified {@code value}.
+     */
+    public static Slug create(String value) {


If you name these methods repoSlug(), orgSlug(), and newSlug(), you'll be able to use static imports and avoid the name of the utility class. So, instead of

var s = Slugs.forRepo(value);

... you'll get:

var s = repoSlug(value);

... which sounds a bit more English.

alexander-yevsyukov · 2020-07-15T18:09:15Z

google-chat-bot/src/main/proto/spine/chatbot/github/slug.proto

+    //
+    // E.g. it could be a GitHub repository slug in the format `SpineEventEngine/base`.
+    //
+    string value = 1 [(required) = true];


Can we have a regexp option here, please?

I don't think there is a particular regexp to cover all the possible variants. The slug can be an organization slug or a repository slug or some other slug as well. Basically, as stated, it is a human-readable identifier.

It may happen that we have `build` as a package name in Java, thus with such a wildcard ignore all the files in the package are also ignored automatically.

The prefix was removed accidentally as part of the `api` package removal.

yuri-sergiichuk self-assigned this Jun 9, 2020

yuri-sergiichuk requested a review from armiol June 9, 2020 15:34

armiol requested changes Jun 9, 2020

View reviewed changes

armiol requested changes Jun 17, 2020

View reviewed changes

yuri-sergiichuk requested a review from armiol June 19, 2020 16:54

armiol requested changes Jun 20, 2020

View reviewed changes

yuri-sergiichuk mentioned this pull request Jun 20, 2020

Consider implementing retry/backoff policy #4

Open

yuri-sergiichuk requested a review from armiol June 21, 2020 14:39

yuri-sergiichuk added 3 commits June 22, 2020 16:42

Use our own state do determine whether a build is recovered or stable.

18472cb

Trigger builds as recovered only if we previously had them as failed in the process.

use info from the RepoBranchBuildResponse and not only from the build…

0fd30f7

… itself. E.g. grab Repository and branch name from the response

Fix capitalization

a32f86a

alexander-yevsyukov requested changes Jun 22, 2020

View reviewed changes

yuri-sergiichuk added 15 commits June 22, 2020 20:59

Add missing EOLs

99cf916

Shorten initializeEnvironment to init.

7a1e893

rename instance to newInstance

fb91c31

Extract logger

a13a999

Drop repetitive inheritance comment

19938d7

Drop Google Chat part of the doc

5f4f3aa

Cleanup doc

375ae8b

Mention the entry point and env config

981c004

Drop idOf suffix from Identifiers

37a8ed0

Use OrgId for logs and URLs

5f965fe

Use string constant directly

85d2ae7

Move docs to the class level and extract help methods.

f065a46

Rename API clients.

3298c63

Add method-level doc

3e67870

Drop extra empty lines

ee059d5

yuri-sergiichuk added 10 commits July 15, 2020 17:23

Use {@linkplain} where appropriate. Fix misprints.

0492d62

Wrap param names into {@code}

bb57282

Add @see link to the respective docs

00a0465

Fix article usage

aa1a1ff

Cleanup docs

4593b48

User shorter name for the repository

6ffe9b1

Fix capitalization

7025930

Improve docs

a93794b

Drop usage of the shortcut javadocs notation

15d86a6

Move identifiers to common packages in order to use them both in clie…

8ba7ceb

…nt and server code

yuri-sergiichuk requested review from armiol and alexander-yevsyukov July 15, 2020 15:08

alexander-yevsyukov reviewed Jul 15, 2020

View reviewed changes

google-chat-bot/src/test/java/io/spine/chatbot/CanFailFast.java Show resolved Hide resolved

Add missing test

2ac05e4

alexander-yevsyukov approved these changes Jul 15, 2020

View reviewed changes

yuri-sergiichuk added 12 commits July 15, 2020 22:04

Add non-null checks

6efcf33

Simplify string2enum conversion

1cabba9

Start potentially long data with an empty line

b28e3c1

Improve Slugs methods naming

35e99e5

Add ability to query org aggregate

f9caa16

Do not ignore *all* build folders.

2c8c26c

It may happen that we have `build` as a package name in Java, thus with such a wildcard ignore all the files in the package are also ignored automatically.

Fix env variable name

6d0e07a

Fix local credentials path

693dcb2

Bring back api prefix of the Travis API base URL.

2ba2fa7

The prefix was removed accidentally as part of the `api` package removal.

Enable trace-level logs

0c36b06

Add diagnostic event logger for both bounded contexts.

2929602

Bump version to 1.0.0

e46dd3e

yuri-sergiichuk merged commit b525f06 into master Jul 16, 2020

yuri-sergiichuk deleted the initial-chat-implementation branch July 16, 2020 17:27

		@@ -0,0 +1,24 @@
		# TODO:2020-06-17:yuri-sergiichuk: the build fails in the cloud with

	* string representation.
	* string representation, ignoring the case.

Initial ChatBot implementation #1

Initial ChatBot implementation #1

Conversation

yuri-sergiichuk commented Jun 9, 2020 • edited

yuri-sergiichuk commented Jun 9, 2020

armiol left a comment

Choose a reason for hiding this comment

armiol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuri-sergiichuk commented Jun 19, 2020

armiol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuri-sergiichuk commented Jun 21, 2020

alexander-yevsyukov left a comment

Choose a reason for hiding this comment

yuri-sergiichuk commented Jul 15, 2020

alexander-yevsyukov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuri-sergiichuk commented Jun 9, 2020 •

edited