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.

Sign up for GitHub

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

Jump to bottom

MonitorSchedule constructor that validates crontab syntax #625

Merged
merged 7 commits into from
Nov 10, 2023
Merged

MonitorSchedule constructor that validates crontab syntax #625

merged 7 commits into from
Nov 10, 2023

Conversation

szokeasaurusrex
Copy link
Member

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

@szokeasaurusrex szokeasaurusrex self-assigned this Nov 8, 2023
@szokeasaurusrex szokeasaurusrex mentioned this pull request Nov 8, 2023
Merged
@codecov Codecov
Copy link

codecov bot commented Nov 8, 2023 •
edited

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

@szokeasaurusrex 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
Swatinem reviewed Nov 9, 2023
View reviewed changes
sentry-types/src/crontab_validator.rs Outdated
.map(CronToken::Numeric)
.chain(MONTHS.iter().map(|&month| CronToken::Alphabetic(month)))
.collect(),
(0..8)
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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).

sentry-types/src/crontab_validator.rs Outdated
}

pub fn validate(crontab: &str) -> bool {
let allowed_values = vec![
Copy link
Member

Choose a reason for hiding this comment

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

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

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 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.

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 rewrote this a bit; now the allowed values are defined in a constant

sentry-types/src/crontab_validator.rs Outdated
Comment on lines 42 to 47
let range = match steprange_split.next() {
Some(range) => range,
None => {
return false;
}
};
Copy link
Member

Choose a reason for hiding this comment

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

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

@szokeasaurusrex szokeasaurusrex marked this pull request as ready for review November 9, 2023 14:49
@szokeasaurusrex
Copy link
Member Author

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?

@szokeasaurusrex szokeasaurusrex changed the title [very preliminary draft] MonitorSchedule constructor that validates crontab syntax MonitorSchedule constructor that validates crontab syntax Nov 9, 2023
Swatinem
Swatinem approved these changes Nov 10, 2023
View reviewed changes
Copy link
Member

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

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

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
loewenheim approved these changes Nov 10, 2023
View reviewed changes
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 to me, apart from the MSRV incompatible changes.

@szokeasaurusrex szokeasaurusrex enabled auto-merge (squash) November 10, 2023 13:15
@szokeasaurusrex
Copy link
Member Author

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?

@szokeasaurusrex szokeasaurusrex merged commit da4ff66 into master Nov 10, 2023
12 checks passed
@szokeasaurusrex szokeasaurusrex deleted the szokeasaurusrex/validate-crontab branch November 10, 2023 13:45
jan-auer added a commit that referenced this pull request Dec 11, 2023
@jan-auer
Merge branch 'master' into swatinem/metrics
255c578 
* 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Swatinem Swatinem Swatinem approved these changes

@loewenheim loewenheim loewenheim approved these changes

Assignees

@szokeasaurusrex szokeasaurusrex

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@szokeasaurusrex @Swatinem @loewenheim