Skip to content
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.

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

Event bus and websocket endpoint #404

Merged
merged 3 commits into from
Dec 3, 2018
Merged

Event bus and websocket endpoint #404

merged 3 commits into from
Dec 3, 2018

Conversation

edvald
Copy link
Collaborator

@edvald edvald commented Nov 30, 2018

Added an event bus on garden.events, and just to kick things off I'm emitting some events from the TaskGraph.

The WebSocket endpoint forwards any event emitted to garden.events to open
connections, and allows commands to be executed via the socket as well.

@eysi09
Copy link
Collaborator

eysi09 commented Dec 1, 2018

Could you add instructions for how to interact with the API endpoints? E.g. the shape of the JSON payload, the command.subCommand syntax. Doesn't have to be detailed, just some examples one can copy to get started.

@@ -22,7 +22,7 @@ import { Module } from "../types/module"
import { logHeader } from "../logger/util"

const buildArguments = {
module: new StringsParameter({
modules: new StringsParameter({
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we're changing this to plural, shouldn't it be updated for all the command arguments? We generally use the singular module and service even though we accept a list of them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, we can update the rest of them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added a commit for that.

const parts = env.split(".")
const environmentName = parts[0]
const parts = environmentName.split(".")
environmentName = parts[0]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is slightly confusing (although not introduced in this PR).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yea, I've actually already removed this in another branch.

* Recursively process all values in the given input,
* walking through all object keys _and array items_.
*/
export function deepMap<T extends object, U extends object = T>(
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't appear to be used anywhere.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

True, I copied it in for reference for the deepFilter function, and figured it might as well stay.

The endpoint forwards any event emitted to `garden.events` to open
connections, and allows commands to be executed via the socket as well.
@@ -24,7 +24,7 @@ import dedent = require("dedent")

const callArgs = {
serviceAndPath: new StringParameter({
help: "The name(s) of the service(s) to call followed by the ingress path (e.g. my-container/somepath).",
help: "The name(s) of the service to call followed by the ingress path (e.g. my-container/somepath).",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why remove the plural (s) from service(s)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can't call more than one service here.

Copy link
Collaborator

@eysi09 eysi09 Dec 3, 2018

Choose a reason for hiding this comment

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

Ah yes. So name should also be singular then.

Copy link
Collaborator

Choose a reason for hiding this comment

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

...I'll go ahead and fix it and merge.

This should have no impact on CLI usage.
@eysi09 eysi09 merged commit 5148cdb into master Dec 3, 2018
@eysi09 eysi09 deleted the event-bus branch December 3, 2018 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants