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

[gh-24311] Expand on documentation about jobs that are both parameterised and periodic #24384

Merged
merged 19 commits into from
Nov 8, 2024

Conversation

Juanadelacuesta
Copy link
Member

@Juanadelacuesta Juanadelacuesta commented Nov 7, 2024

@Juanadelacuesta Juanadelacuesta marked this pull request as ready for review November 7, 2024 12:59
@Juanadelacuesta Juanadelacuesta changed the title docs: expand on documentation about jobs that are both parameterised … Expand on documentation about jobs that are both parameterised and periodic Nov 7, 2024
@Juanadelacuesta Juanadelacuesta changed the title Expand on documentation about jobs that are both parameterised and periodic [gh-24311] Expand on documentation about jobs that are both parameterised and periodic Nov 7, 2024
@jrasell jrasell requested a review from aimeeu November 7, 2024 13:07
@Juanadelacuesta Juanadelacuesta added backport/1.8.x backport to 1.8.x release line backport/1.9.x backport to 1.9.x release line backport/1.7.x backport to 1.7.x release line backport/ent/1.7.x+ent Changes are backported to 1.7.x+ent backport/ent/1.8.x+ent Changes are backported to 1.8.x+ent labels Nov 7, 2024
@aimeeu aimeeu added the backport/website This will backport PR changes to `stable-website` && the latest release-branch label Nov 7, 2024
Copy link
Member

@gulducat gulducat left a comment

Choose a reason for hiding this comment

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

looks really good! just a few comments to consider :)

website/content/docs/job-specification/parameterized.mdx Outdated Show resolved Hide resolved
ID Type Submit Date
sync batch/periodic/parameterized 2024-11-07T10:43:30+01:00 // Original submitted job
sync/dispatch-1730972650-247c6e97 batch/periodic 2024-11-07T10:44:10+01:00 // First dispatched job with parameters A
sync/dispatch-1730972650-247c6e97/periodic-1730972680 batch 2024-11-07T10:44:40+01:00 // Cron job with parameters A
Copy link
Member

Choose a reason for hiding this comment

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

whoa! I didn't notice these great // comments in the preview, because I didn't see that the little code block could scroll to the right; I only saw to the Type column. I'm not sure what to suggest for that... maybe say above this to "Scroll to the right for descriptions of each job."?

Comment on lines 62 to 64
See the [parameterized] documentation for additional considerations when
using in conjunction with parameterized options.

Copy link
Member

Choose a reason for hiding this comment

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

this seems a little buried under these Parameters. maybe move it up under Requirements?

Copy link
Contributor

Choose a reason for hiding this comment

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

Since this periodic page doesn't talk about parameterized options at all, what about moving this line right before the # periodic Requirements section?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is a bit of a corner case, I didn't want to put it in the general description

Copy link
Contributor

@aimeeu aimeeu left a comment

Choose a reason for hiding this comment

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

Thanks for the changes! A few more suggestions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/ent/1.7.x+ent Changes are backported to 1.7.x+ent backport/ent/1.8.x+ent Changes are backported to 1.8.x+ent backport/website This will backport PR changes to `stable-website` && the latest release-branch backport/1.7.x backport to 1.7.x release line backport/1.8.x backport to 1.8.x release line backport/1.9.x backport to 1.9.x release line
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants