Validate --event-time-start is before --event-time-end
#10820
Conversation
…ed from CLI Sometimes CLI options have restrictions based on other CLI options. This is the case for `--event-time-start` and `--event-time-end`. Unfortunately, click doesn't provide a good way for validating this, at least not that I found. Additionaly I'm not sure if we have had anything like this previously. In any case, I couldn't find a centralized validation area for such occurances. Thus I've gone and added one, `validate_option_interactions`. Long term if more validations are added, we should add this wrapper to each CLI command. For now I've only added it to the commands that support `event_time_start` and `event_time_end`, specifically `build` and `run`.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #10820 +/- ##
==========================================
- Coverage 89.20% 89.19% -0.01%
==========================================
Files 183 183
Lines 23402 23416 +14
==========================================
+ Hits 20875 20887 +12
- Misses 2527 2529 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Can you please move flag validation logic here to be consistent with other flags? That'll avoid the need to create a new Click option as well.
dbt-core/core/dbt/cli/flags.py
Line 311 in 75a0962
core/dbt/cli/flags.py
Outdated
| raise DbtUsageException( | ||
| "Value for `--event-time-start` must be less than `--event-time-end`" | ||
| ) | ||
| elif event_time_start >= datetime.now(): |
There was a problem hiding this comment.
These are two separate conditions, so let's do two ifs?
Also, do we need to specify UTC timezone for datetime.now()?
There was a problem hiding this comment.
To the first part, they are not two separate ifs. This was intentional as the commit message for ee994c1 states. We only need to do the second check if event_time_end is not set. Said another way, the relation of event_time_start to datetime.now() is irrelevant if event_time_end is set, and thus doesn't need to be checked.
As for the second part, maybe? The issue is that the click parsing of datetimes creates naive datetimes (lacking timezone information). It won't even parse something like --event-time-start "2024-10-07 13:45:00+00:00". It'll give you an error like:
Invalid value for '--event-time-start': '2024-10-07 00:00:00+00:00' does not match the formats '%Y-%m-%d', '%Y-%m-%dT%H:%M:%S', '%Y-%m-%d %H:%M:%S'.
So our options are to either assume what they give us is UTC, and give the passed in --event-time-start a timezone, or compare to a naive datetime. Given the rest of our system assumes UTC, you are right, we should probably do the former instead of what we are currently doing (the latter).
core/dbt/cli/flags.py
Outdated
| event_time_start = ( | ||
| getattr(self, "EVENT_TIME_START") if hasattr(self, "EVENT_TIME_START") else None | ||
| ) | ||
| if event_time_start is not None: |
There was a problem hiding this comment.
Nit: do we need this level of nesting?
There was a problem hiding this comment.
I think so? We need to check whether or not event_time_start is None, as it only makes sense to do the validation if event_time_start is a datetime value. We could get around this layer of nesting by doing something like:
if not hasattr(self, "EVENT_TIME_START"):
return
event_time_start = getattr(self, "EVENT_TIME_START")
...I'm personally not a huge fan of multiple returns. I can see how doing an early return would reduce our level of if's from 2 down to 1. However, given we're only at 2 levels of if's that seems unnecessary.
Side note that's not applicable here, but a corollary to my above "I avoid multiple returns".
If there were more levels of nesting, I still likely woudn't add early returns. I'd instead end up looking at splitting it into multiple functions, thus continuing allowing to maintain low levels of if context nesting and avoiding early multiple returns in a function.
QMalcolm
deleted the
qmalcolm--10786-validate-event-start-time-end-start-relationship
branch
Resolves #10786
Problem
A person could specify an
--event-time-startthat was after--event-time-endor the current time. This would cause bad things to happen ™️Solution
Validate the cli options such that
--event-time-startis before--event-time-end--event-time-startis before the current time if--event-time-endis not setChecklist