-
Notifications
You must be signed in to change notification settings - Fork 89
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.
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
[COST-4750] OCP Scrub disabled tags in trino. #4961
Conversation
# remove disabled tag keys | ||
sql_params = { | ||
"start_date": self.first_start_date, | ||
"end_date": self.last_end_date, | ||
"report_period_ids": report_period_ids, | ||
"schema": self.schema, | ||
} | ||
sql = pkgutil.get_data("api.report.test.util.mock_trino_sql", "ocp/remove_disabled_tags.sql") | ||
sql = sql.decode("utf-8") | ||
table_name = accessor._table_map["line_item_daily_summary"] | ||
accessor._prepare_and_execute_raw_sql_query(table_name, sql, sql_params) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the purpose of this change. Why not just leave this functionality where it is and just update the names and the docstring to indicate that it is used for unit testing? The purpose of the accessor is to run sql against the db, so we should leave that functionality there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just sent a message to the team chat about this 😂 The reason for the change is that I would like to disconnect these files from our data processing flow. I want to make clear that the only purpose this file serves now is for unit testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am open to suggestions though, if we want to keep it in the accessor that is fine. I just want a cleaner way of understanding which sql files are being used for unit-testing purposes only. But that could just mean moving them to a separate directory.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #4961 +/- ##
=====================================
Coverage 94.0% 94.0%
=====================================
Files 375 375
Lines 31116 31117 +1
Branches 3697 3697
=====================================
+ Hits 29264 29265 +1
Misses 1181 1181
Partials 671 671 |
/retest |
/retest |
3 similar comments
/retest |
/retest |
/retest |
Tests are failing on bad sql. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job Cody
Jira Ticket
COST-4750
Description
This change will move the scrubbing of disabled tags up into the trino flow.
Testing
Snip it of the original SQL:
Snip it of the new SQL:
You can compare these.
Another way to test is to just set all OCP tags as disabled, and refresh. You should now see no pod_labels in the daily summary table in postgresql.
Notes
...