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

Serverless emit #4038

Merged
merged 7 commits into from Aug 9, 2017
Merged

Serverless emit #4038

merged 7 commits into from Aug 9, 2017

Conversation

nikgraf
Copy link
Contributor

@nikgraf nikgraf commented Aug 4, 2017

What did you implement:

Closes #4023

How did you implement it:

The priority of the parameters as wells as how data is based on invoke, but I polished it a bit more here. To emit the events we use the FDK.

How can we verify it:

Spin up a server accepting an HTTP endpoint, spin up an event gateway, register the function and subscription and run serverless emit

Todos:

  • Write tests
  • Write documentation
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

@nikgraf
Copy link
Contributor Author

nikgraf commented Aug 4, 2017

@brianneisler @eahefnawy @pmuens what are our plans regarding documentation on serverless run and serverless emit?

Copy link
Member

@eahefnawy eahefnawy left a comment

Choose a reason for hiding this comment

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

Awesome stuff @nikgraf ... Love the attention to details, specially when it comes to logging and error handling.

Added some comments about the data, and haven't tested it yet. I'll run an inclusive test of sls run and sls emit

this.data = JSON.parse(this.options.data);
resolve();
} catch (exception) {
reject(new Error("Couldn't parse the provided data to a JSON structure."));
Copy link
Member

Choose a reason for hiding this comment

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

data can't be simply strings? is JSON a requirement of the gateway?

Copy link
Contributor

Choose a reason for hiding this comment

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

Data is not always JSON. We will need to support other data formats eventually.

this.data = JSON.parse(input);
resolve();
} catch (exception) {
reject(new Error("Couldn't parse the provided data to a JSON structure."));
Copy link
Member

Choose a reason for hiding this comment

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

same comment. I think in invoke/local we catch and ignore, we don't throw an error.

}

emitEvent() {
userStats.track('service_emitted');
Copy link
Member

Choose a reason for hiding this comment

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

❤️ ❤️ ❤️ ❤️ ❤️ ❤️

I should add that to serverless run too!

return eventGateway
.emit({
event: name,
data: JSON.stringify(data),
Copy link
Member

Choose a reason for hiding this comment

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

hmmm so we JSON.parse, then we JSON.stringify? can't we just pass the data as it is?

usage: 'input data',
shortcut: 'd',
},
url: {
Copy link
Member

Choose a reason for hiding this comment

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

I think the url is always local host, imo it'd be better DX to just provide the port.

Actually, ideally we'd be providing all of that via serverless.yml, but we decided not to do that yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's localhost for now, but if you want to emit to a hosted gateway it will be a different url. We use localhost:4000 as default, but this is a feature already optimised to be future-proof. I suggest to keep url

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, this should remain url. We will need a way of specifying the url of the gateway when it is hosted on the cloud.

@nikgraf
Copy link
Contributor Author

nikgraf commented Aug 4, 2017

Just had a call with the team. We will try to parse JSON and if it works we send the content type json. If it doesn't we send the content-type text.

To provide good UX we print out the structure and content type. This also can be improved in invoke.

lifecycleEvents: ['emit'],
options: {
name: {
usage: 'Event name',
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we rename this to type please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done 👍

@eahefnawy
Copy link
Member

tried it out with the following command:

sldev emit -n user.created -d "{}"

and I got the following in event gateway logs:

INFO    Running in development mode with embedded etcd.
DEBUG   Event received. {"event": "{\"event\":\"user.created\",\"id\":\"57b92399-1fe7-43df-b50f-2c1aa06a64c0\",\"receivedAt\":3325049972,\"data\":{\"data\":{}},\"dataType\":\"application/json\"}"}

Thanks @nikgraf 🙌

merging... ✨

@eahefnawy eahefnawy merged commit 8a51032 into master Aug 9, 2017
@pmuens pmuens deleted the serverless-emit branch August 9, 2017 09:39
@pmuens pmuens added this to the 1.20 milestone Aug 9, 2017
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.

None yet

4 participants