Core storage model cost [VS-473] #7913
Conversation
Codecov Report
@@ Coverage Diff @@
## ah_var_store #7913 +/- ##
================================================
Coverage ? 86.286%
Complexity ? 35188
================================================
Files ? 2170
Lines ? 164888
Branches ? 17786
================================================
Hits ? 142276
Misses ? 16289
Partials ? 6323 |
| @@ -2,11 +2,11 @@ version 1.0 | |||
|
|
|||
| workflow GvsCallsetCost { | |||
| input { | |||
| # String project_id | |||
| # String dataset_name | |||
| String project_name | |||
There was a problem hiding this comment.
Why "project_name"? All other WDLs use "project_id", I believe.
There was a problem hiding this comment.
I'll fix this, I actually confused myself with the three different ways of identifying a Google project.
|
|
||
| # Technically single quotes and exclamation points are allowed but none of that nonsense here. | ||
| # https://cloud.google.com/resource-manager/docs/creating-managing-projects#:~:text=A%20project%20name%20can%20contain,between%204%20and%2030%20characters. | ||
| valid='-_0-9a-zA-Z' |
| String workspace_namespace | ||
| String workspace_name | ||
| # String callset_name | ||
| String callset_name |
There was a problem hiding this comment.
what is the callset name? is this the same as the filter_set_name ?
There was a problem hiding this comment.
Actually this should be callset identifier, which is the name of a column in the cost observability table. It looks like the plan is to default the filter set name to the call set identifier unless a value is explicitly specified.
mcovarr
force-pushed
the
vs_473_core_storage_model_cost
branch
from
2487afb to
45da559
Compare
| String workspace_namespace | ||
| String workspace_name | ||
| # String callset_name | ||
| String callset_identifier |
There was a problem hiding this comment.
nit: change to call_set_identifier to be consistent with table field?
There was a problem hiding this comment.
yeah this was bugging me too... I was wondering whether to standardize on call_set or callset, sounds like you prefer the former.
| } | ||
| command <<< | ||
|
|
||
| sanity_check_project() { |
There was a problem hiding this comment.
Why validate the project id? And if there are good reasons to validate it, why just here?
There was a problem hiding this comment.
This gets interpolated into various BQ queries and given all the recent Hack EDU training I thought a little validation might be in order. 🙂 I was working on centralizing all input validation to one task in my next PR with downstream tasks only running if that succeeds.
There was a problem hiding this comment.
actually since I have reviewers' attention now I'll just mosey that code on from my in-progress branch to this one... 🙂
| fi | ||
| } | ||
|
|
||
| sanity_check_dataset_name() { |
There was a problem hiding this comment.
see questions about sanity_check_project()
No description provided.