-
Notifications
You must be signed in to change notification settings - Fork 18
docs: update cron trigger runs description #711
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
Merged
Merged
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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.
Is this appropriate for this section of docs?
We're deep in an explanation of state here. I don't think this is the time or place to explain what a cron job is. We could add a single sentence definition, but otherwise we should link to the full cron docs.
Otherwise I think this is just a distraction from the discussion on this page.
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.
This feels like the main docs? https://docs.openfn.org/documentation/build/triggers#cron-triggers-formerly-timers
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.
@josephjclark In the link you have shared, the explanation still links to the docs we are currently updating to explain about state in cron jobs.
If readers are being directed to these docs, should we go deeper into state in cron jobs?
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.
But that's only for state specifically.
"What is a cron job and how does it work" needs to be answered on
build/triggers. I think those docs are fine right now but I'm open to discussing improvements."What is the input state passed into my cron job" needs to be explained in-depth in jobs/state. This stuff about "the output of the trigger step will be the input to the next step". It might need a concerete worked example to explain it.
build/triggerscan have a single-sentence explanation of how state works, then link tojobs/statefor more detailThere 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 have explained more about the state in
jobs/statewith the new discovery of how state works in cronjobsThere 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.
@hunterachieng this still isn't right :(
The
state.mdpage does NOT explain what Webhook or Kafka triggered runs are. Why are we treating Cron runs differently?This page is explaining STATE. Not CRON. It assumes you know what a cron triggered run is because why else would you be here?
A single sentence explanation is maybe OK but honestly I think this whole
what are cron jobssection needs to go. You can link tobuild/triggersif you really want.