Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[spin]: Rework trigger docs #1450
base: main
Are you sure you want to change the base?
[spin]: Rework trigger docs #1450
Changes from 1 commit
1d48460
88967be
887f8f0
453e9a3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
time-driven? Possible copypasta from cron?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am uncomfortable with "pre-written." "Template" or "starter" or "generated", but it's not really "pre-written".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to include a troubleshooting/gotchas section to cover how this doesn't play nice with multi-trigger applications?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the command trigger that is definitely super important and useful. Will add a corresponding paragraph
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure what distinguishes a "common" trigger from a "community" one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought separation would make sense. Although
mqtt
is mostly written by fermyon ppl, its not under the fermyon org.If we don't care, we could roll with just a single (simple) list of all triggers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we just need to rephrase "common" to align more with "Fermyon"? It currently sounds like it contrasts with "rare" which is clearly not your intent!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wary of repeating this list on every trigger page. Could we maybe instead have a "See all Spin triggers" link to a top level page? Or nerd snip Karthik into giving us some kind of 'include from file' feature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure this needs to be in the title. It might work better in a boxout at the top of the page where you can explain the significance a la "not maintained by Fermyon" etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we expand on what "runs based on real time data records" means?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could clarify this by saying something like "parses data records streamed through AWS Kinesis with customizable processing delay" -- but I wonder if it gets into the weeds a bit too much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going into the details of credential management seems a bit out of sync with this high level. Maybe a "configuring the Kinesis trigger" or "Kinesis trigger authentication and authorization" section further down?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, I've almost no experience with AWS. Even the trigger repo does not provide further info
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ogghead would you have a moment to look over this and share any thoughts on how to structure the Kinesis trigger docs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Knowing that an existing Kinesis stream and IAM credentials with permissions to read from it are prerequisites has benefit to readers if they are unfamiliar with AWS/are not ready to set those up. But it does also get into the weeds a bit.
Perhaps this information could live in a "prerequisites" subsection under "Installing the AWS Kinesis Trigger" before the sections on plugin installation/Spin configuration? Flexible on approach overall!
Unrelated, but it is potentially worth linking this AWS doc on credentials on "leverages AWS credentials from the standard AWS configuration environment variables" to help folks familiarize themselves with AWS credentials options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For each batch rather than for each record? The iteration is internal to "the function."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good callout @itowlson, agreed it might be worth rewording to clarify that function execution is per batch of records