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

feat(api): Support monitor upserts #1807

Merged
merged 13 commits into from
Nov 13, 2023
Merged

Conversation

szokeasaurusrex
Copy link
Member

@szokeasaurusrex szokeasaurusrex commented Nov 8, 2023

The Sentry CLI can now support creating cron monitors while executing the monitors run command via the new --schedule or the equivalent abbreviated -s argument. The argument expects to receive the schedule in crontab syntax, which the CLI validates. This PR also introduces the --check-in-margin, --max-runtime, and --timezone arguments to allow for additional configuration of the cron monitor.

Closes #1801

src/commands/monitors/run.rs Outdated Show resolved Hide resolved
src/commands/monitors/run.rs Outdated Show resolved Hide resolved
src/commands/monitors/run.rs Outdated Show resolved Hide resolved
@szokeasaurusrex
Copy link
Member Author

None of the cron parsing crates I could find seemed very good, so I wrote my own cron parser and opened a draft PR for it here: getsentry/sentry-rust#625. I probably need to do some polishing up on that PR, but once we are able to merge it, we can use that code to validate the crontab schedule provided by the user

@loewenheim
Copy link
Contributor

I agree with the decision to do this ourselves, though I'm normally more inclined to just grab something off the rack, as it were. But all the crates for this crontab stuff are ancient.

@szokeasaurusrex
Copy link
Member Author

I agree with the decision to do this ourselves, though I'm normally more inclined to just grab something off the rack, as it were. But all the crates for this crontab stuff are ancient.

100% agree with your point; I would have also preferred to use a crate if a suitable one existed. Maybe we can consider publishing the cron validator I wrote as its own crate so others can use it

Co-authored-by: Sebastian Zivota <[email protected]>
@szokeasaurusrex szokeasaurusrex changed the title Support monitor upserts [Do not merge yet] Support monitor upserts Nov 9, 2023
Cargo.toml Outdated
Comment on lines 61 to 62
# TODO: Revert back to crates.io version prior to merge
sentry = {git = "https://github.com/getsentry/sentry-rust.git", rev = "refs/pull/625/head", default-features = false, features = [
Copy link
Member Author

Choose a reason for hiding this comment

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

I have pinned here my pull request on sentry-rust, which adds the crontab validation, so that we can test this PR before the sentry-rust changes are merged.

We need to merge the sentry-rust PR before merging this PR and release a new sentry-rust version, and we should pin the new version here before merging.

@szokeasaurusrex szokeasaurusrex changed the title [Do not merge yet] Support monitor upserts Support monitor upserts Nov 13, 2023
@szokeasaurusrex szokeasaurusrex marked this pull request as ready for review November 13, 2023 10:11
@szokeasaurusrex szokeasaurusrex enabled auto-merge (squash) November 13, 2023 10:11
@szokeasaurusrex szokeasaurusrex changed the title Support monitor upserts feat(api): Support monitor upserts Nov 13, 2023
@szokeasaurusrex szokeasaurusrex marked this pull request as draft November 13, 2023 10:27
Copy link
Contributor

@loewenheim loewenheim left a comment

Choose a reason for hiding this comment

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

Looks good apart from my one comment :)

src/commands/monitors/run.rs Outdated Show resolved Hide resolved
@szokeasaurusrex szokeasaurusrex marked this pull request as ready for review November 13, 2023 13:25
@szokeasaurusrex szokeasaurusrex enabled auto-merge (squash) November 13, 2023 13:26
@szokeasaurusrex szokeasaurusrex merged commit 0b42f79 into master Nov 13, 2023
16 checks passed
@szokeasaurusrex szokeasaurusrex deleted the szokeasaurusrex/crons-upsert branch November 13, 2023 13:30
@evanpurkhiser
Copy link
Member

@szokeasaurusrex would you also be able to update the CLI documentation to include details about how to use this

@szokeasaurusrex
Copy link
Member Author

@evanpurkhiser I opened an issue so I remember to add these docs; I should get around to adding it later this week.

@evanpurkhiser
Copy link
Member

Perfect. Thank you!

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.

Crons monitor upsert support in sentry-cli
3 participants