Support excluding time intervals during storage invoicing and fixed typo #174
Conversation
src/coldfront_plugin_cloud/management/commands/calculate_storage_gb_hours.py
Outdated
Show resolved
Hide resolved
Nevermind the above comment. I wrote it and then forgot to delete it. |
QuanMPhm
force-pushed
the
166/exclude_intervals
branch
from
918658a
to
4dca5d9
Compare
src/coldfront_plugin_cloud/management/commands/calculate_storage_gb_hours.py
Show resolved
Hide resolved
src/coldfront_plugin_cloud/tests/unit/test_calculate_quota_unit_hours.py
Outdated
Show resolved
Hide resolved
QuanMPhm
force-pushed
the
166/exclude_intervals
branch
from
4dca5d9
to
237baf9
Compare
src/coldfront_plugin_cloud/utils.py
Outdated
| [ | ||
| datetime.datetime.strptime(start, "%Y-%m-%d"), | ||
| datetime.datetime.strptime(end, "%Y-%m-%d").replace( | ||
| hour=23, minute=59, second=59) |
There was a problem hiding this comment.
Apologies, I should have caught this in earlier iterations but it only really hit me yesterday evening.
This is inconsistent with how we are treating dates in the rest of this script.
Running the application with --start 2024-01-01 and --end 2024-01-02 will produce a 24 interval (from midnight to midnight), however an --excluded-date-ranges 2024-01-01,2024-01-02 would be a 48 hour interval.
Regardless of which is the better way, please treat dates as meaning 00:00 on that date to be consistent.
There was a problem hiding this comment.
I see. Sorry for not realizing this design choice as well when implementing the feature.
QuanMPhm
force-pushed
the
166/exclude_intervals
branch
from
237baf9
to
55ee00f
Compare
| e_interval_end = _clamp_time(e_interval_end, start, end) | ||
| total_interval_duration -= (e_interval_end - e_interval_start).total_seconds() | ||
|
|
||
| return math.ceil(total_interval_duration) |
There was a problem hiding this comment.
@knikolla it's possible that total_interval_duration could become negative if the user inadvertently supplies multiple overlapping intervals to exclude. That's obviously user error but it'd still be nice to add an assertion that total_interval_duration >=0
There was a problem hiding this comment.
@naved001 I have solved a medium Leetcode problem and added a function that will merge overlapping excluded time intervals
|
@QuanMPhm have you had a chance to review Naved's suggestions? |
QuanMPhm
force-pushed
the
166/exclude_intervals
branch
from
55ee00f
to
481e988
Compare
src/coldfront_plugin_cloud/utils.py
Outdated
| time = max_time | ||
| return time | ||
|
|
||
| def _merge_date_ranges(intervals_list: list): |
There was a problem hiding this comment.
I appreciate your enthusiasm in writing code to support this, but this then means that accepting overlapping intervals as an argument is a supported feature of the codebase, which we don't want to be the case.
Some explanations as to why:
- The admin running the command will not be incentivized to fix the way they are storing these intervals.
- It may also be the case that some other tools we're using do not implement these merging function, therefore using the same interval on OpenStack billing for example will return an error, but not here.
- This is a complex function that we'd have to keep supporting moving forward in cases of bugs.
Please simply check for overlapping intervals or unordered start and end and error in case of malformed input.
There was a problem hiding this comment.
@knikolla Sorry for taking a while to ask this: Since removing support for merging intervals means overlapping intervals will result in faulty billing, I should throw an error for overlapping intervals as well?
I've removed the merging function, and perform assertions for overlaps and unordered start-end intervals.
Added an optional CLI argument which allows passing in a txt file containing excluded time intervals. Each line in the txt file should contain a date range that we want to exclude from billing, formatted as "%Y-%m-%d,%Y-%m-%d". An example line could be "2023-02-01,2023-02-03". The time period excluded from billing starts from the first second of the start date, and ends on the first second of the end date.
QuanMPhm
force-pushed
the
166/exclude_intervals
branch
from
481e988
to
ec453be
Compare
Closes #166. Added an optional CLI argument which allows passing in a txt file containing excluded time intervals. Each line in the txt file should contain a date range that we want to exclude from billing, formatted as
"%Y-%m-%d,%Y-%m-%d". An example line could be "2023-02-01,2023-02-03".
The time period excluded from billing starts from the first second of the start date, and ends on the last second of the end date. This means it is possible for the excluded date range to start and end on the same day.
Also fixed a minor typo.