Add new columns for daily report#171
Conversation
* Remove attributes that we get from coldfront from the invoice. Still keep the CSV output consistent though. * Hold report metadata in a dataclass and pass that around to the helper functions to keep things better contained.
|
Seems good to me. |
naved001
force-pushed
the
columns-for-daily-report
branch
from
99960d1 to
ccb5c35
Compare
Also use a single timestamp for current_time throughout the report.
naved001
force-pushed
the
columns-for-daily-report
branch
from
954b1f1 to
02f2a23
Compare
QuanMPhm
left a comment
QuanMPhm
left a comment
There was a problem hiding this comment.
Just two small comments
openshift_metrics/merge.py
Outdated
| if report_start_date.month != report_end_date.month: | ||
| logger.warning("The report spans multiple months") |
There was a problem hiding this comment.
Should this if clause be removed, since if the extra string is appended, calling nerc_rates with it will raise an date parsing error?
There was a problem hiding this comment.
@QuanMPhm I am not sure I follow why this would cause an error. What extra string are you talking about?
Oh, I see it now. Good point. I will just remove this clause, it's not strictly necessary and now the new columns will indicate the report start and report end anyway.
| su_definitions=su_definitions, | ||
| cluster_name=cluster_name, | ||
| ignore_hours=ignore_hours, | ||
| ) |
There was a problem hiding this comment.
Not a suggestion for this PR, but for a future refactoring if the time ever comes to it.
I think while we're at it refactoring, you can put condensed_metrics_dict, invoice_rates, and a few other arguments into ReportMetadata, or into named more fittingly (I don't know what's a better name :P). The three functions to write the different invoices could probably be refactored too.
There was a problem hiding this comment.
Yes, I have a draft PR that I'll get back to fixing.
|
@naved001 Do you also plan to changing the frequency of the |
The new columns will indicate the report start and end date, so it's not strictly necessary to log a warning anymore. Futhermore, since it modified the report_month string, it'll break the call the to get_su_definitions down the line. I could just move that call to it up, but this warning isn't that necessary anyway.
|
@QuanMPhm The cronjobs that are actually deployed are already up to date, I'll push those to the repository in a different PR. |
3 participants
A bunch of tests will fail, but I want to make sure people are happy with the way things are organized before I update the tests.
The first commit adds the new feature, the 2nd commit refactors things around it.
Closes #166
Related nerc-project/operations#1307