MonitorSchedule constructor that validates crontab syntax by szokeasaurusrex · Pull Request #625 · getsentry/sentry-rust

szokeasaurusrex · 2023-11-08T17:31:47Z

I will want to use the new MonitorSchedule::from_crontab in getsentry/sentry-cli#1807

codecov · 2023-11-08T17:37:53Z

Codecov Report

Merging #625 (16ea158) into master (80e5b13) will decrease coverage by 0.08%.
The diff coverage is 67.07%.

❗ Current head 16ea158 differs from pull request most recent head fb65dfd. Consider uploading reports for the commit fb65dfd to get more accurate results

@@            Coverage Diff             @@
##           master     #625      +/-   ##
==========================================
- Coverage   73.11%   73.03%   -0.08%     
==========================================
  Files          58       59       +1     
  Lines        6576     6658      +82     
==========================================
+ Hits         4808     4863      +55     
- Misses       1768     1795      +27

Swatinem · 2023-11-09T09:48:59Z

sentry-types/src/crontab_validator.rs

+            .map(CronToken::Numeric)
+            .chain(MONTHS.iter().map(|&month| CronToken::Alphabetic(month)))
+            .collect(),
+        (0..8)


with all these ranges, you can use the inclusive range syntax 0..=7, though in this case specifically, shouldn’t it either be 1..=7 or 0..=6? TBH, I’m not that familiar with cron syntax.

Apparently it should be only 0-6 (inclusive). 7 is included in some implementations, but it is nonstandard, so I will remove it from here and go with only 0 through 6.

Inclusive range syntax seems like a good idea though; I was unaware of it

Although I might restructure this now, since I just read that actually the alphabetic abbreviations for days of the week and months are only permitted in standard crontab syntax when they are the only value; they are not permitted to occur within lists or ranges (although some implementations allow it).

Swatinem · 2023-11-09T09:54:08Z

sentry-types/src/crontab_validator.rs

+}
+
+pub fn validate(crontab: &str) -> bool {
+    let allowed_values = vec![


maybe you can use a static once_cell to avoid doing all these allocations every time:
https://docs.rs/once_cell/latest/once_cell/#lazy-initialized-global-data

I think caching the allowed_values vector would be unnecessary for my proposed use case; the sentry-cli would be calling this function at most once per run in order to validate the crontab passed in by the user. So unless we anticipate another use case where we call validate many times per run, I think we can safely keep the function as is.

I rewrote this a bit; now the allowed values are defined in a constant

Swatinem · 2023-11-09T09:55:10Z

sentry-types/src/crontab_validator.rs

+    let range = match steprange_split.next() {
+        Some(range) => range,
+        None => {
+            return false;
+        }
+    };


Suggested change

let range = match steprange_split.next() {

Some(range) => range,

None => {

return false;

}

};

let Some(range) = steprange_split.next() else { return false; };

sentry-types/src/crontab_validator.rs

szokeasaurusrex · 2023-11-09T14:54:22Z

I noticed that CI is currently failing because I am using the function Result::is_ok_and, which was only recently made stable in Rust 1.70.0. However, it looks like CI is running in an older version (1.68.0).

Is upgrading the version of Rust we run against in CI trivial enough that we can make the change here, or do I need to rewrite my code without Result::is_ok_and?

Swatinem

lgtm

though you should maybe change the implementation to still work on our MSRV. In the SDK we are rather conservative with backwards compatibility, and usually bump it if one of the dependencies does so too.

loewenheim

Looks good to me, apart from the MSRV incompatible changes.

szokeasaurusrex · 2023-11-10T13:16:46Z

Okay, I have fixed the MSRV issue.

How soon can we release a new version with these changes, since I would like to use them in getsentry/sentry-cli#1807?

* master: tracing: send spans' attributes along with the event ones (#629) release: 0.32.0 Update crate dependencies (#628) feat: bump http to 1.0.0 (#627) release: 0.31.8 MonitorSchedule constructor that validates crontab syntax (#625) fix(docs): Fix some doc errors that slipped in (#623) docs(tower): Mention how to enable http feature from sentry crate (#622) build(deps): bump rustix from 0.37.23 to 0.37.25 (#619)

MonitorSchedule constructor the validates crontab syntax

2049893

szokeasaurusrex self-assigned this Nov 8, 2023

szokeasaurusrex mentioned this pull request Nov 8, 2023

feat(api): Support monitor upserts getsentry/sentry-cli#1807

Merged

szokeasaurusrex changed the title ~~[very preliminary draft] MonitorSchedule constructor the validates crontab syntax~~ [very preliminary draft] MonitorSchedule constructor that validates crontab syntax Nov 9, 2023

Swatinem reviewed Nov 9, 2023

View reviewed changes

loewenheim reviewed Nov 9, 2023

View reviewed changes

sentry-types/src/crontab_validator.rs Outdated Show resolved Hide resolved

szokeasaurusrex added 5 commits November 9, 2023 14:01

Added tests, and made improvemnts

c721d59

Applied linter fixes

d17c5b4

Deleted commented out code

b08ec77

from_crontab now returns Result

a216277

Implement error for CrontabParseError

be5052f

szokeasaurusrex marked this pull request as ready for review November 9, 2023 14:49

szokeasaurusrex requested review from Swatinem and loewenheim November 9, 2023 14:51

szokeasaurusrex changed the title ~~[very preliminary draft] MonitorSchedule constructor that validates crontab syntax~~ MonitorSchedule constructor that validates crontab syntax Nov 9, 2023

Swatinem approved these changes Nov 10, 2023

View reviewed changes

loewenheim approved these changes Nov 10, 2023

View reviewed changes

Removed is_ok_and call

fb65dfd

szokeasaurusrex enabled auto-merge (squash) November 10, 2023 13:15

szokeasaurusrex merged commit da4ff66 into master Nov 10, 2023

szokeasaurusrex deleted the szokeasaurusrex/validate-crontab branch November 10, 2023 13:45

Uh oh!

Conversation

szokeasaurusrex commented Nov 8, 2023

Uh oh!

codecov bot commented Nov 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

szokeasaurusrex commented Nov 9, 2023

Uh oh!

Swatinem left a comment

Choose a reason for hiding this comment

Uh oh!

loewenheim left a comment

Choose a reason for hiding this comment

Uh oh!

szokeasaurusrex commented Nov 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Nov 8, 2023 •

edited

Loading