reorganize and add timeseries#93
Conversation
krowvin
left a comment
krowvin
left a comment
There was a problem hiding this comment.
Not going to block this if you feel it is sufficient. I did want to KEEP the target office to be able to cross store data for testing purposes. Not a deal breaker, but consider making that option opt-in and keeping it?
Otherwise we can merge to move forward, i'll finalize anything on my end and we can get the rest pulled into main prior to the workshop.
| source_cda: str, | ||
| source_office: str, | ||
| target_cda: str, | ||
| target_office: str, |
There was a problem hiding this comment.
I think we should keep a target office. I would like the option to be able to load data from one office to another.
Would you consider defaulting to the target office being the source office if the target office is not provided?
I think we can add a function/param to the target_office click option to default to the source office?
|
|
||
| if verbose: | ||
| click.echo( | ||
| f"[load location group] source={source_cda} ({source_office}) -> target={target_cda} ({target_office})" |
| for loc in locations: | ||
| click.echo( | ||
| f"[dry-run] would store Location(name={loc.name}) to {target_cda} ({target_office})" | ||
| f"[dry-run] would store Location(name={loc.name}) to {target_cda} ({source_office})" |
| ) | ||
| return | ||
|
|
||
| if not target_api_key and os.getenv("CDA_API_KEY"): |
There was a problem hiding this comment.
Assuming this is removed because we handle the logic at the click argument level? I think I added this because I was not getting an error. We may want to try running this without the key set now to confirm.
There was a problem hiding this comment.
I don't want a default to something that might be in environment variables. someone could accidently copy things into their production database if they run from the T7 and have those variable set. the user needs to purposely set the api key and urls.
There was a problem hiding this comment.
I only meant with target office because it appears you are removing it which means target would always be source?
| )(f) | ||
| f = click.option( | ||
| "--target-office", | ||
| envvar="CDA_TARGET_OFFICE", | ||
| required=True, | ||
| help="Target office ID for writes.", | ||
| )(f) | ||
| f = click.option( |
There was a problem hiding this comment.
Same as above, consider defaulting to source-office. Be able to cross load data.
| @timeseries.command( | ||
| "ids-all", | ||
| help="Copy ALL timeseries IDs for locations in a target CDA from a source CDA.", | ||
| ) | ||
| @shared_source_target_options | ||
| @click.option( | ||
| "--timeseries-id-regex", | ||
| "timeseries_id_regex", | ||
| default=None, | ||
| type=str, | ||
| help="regex filter for timeseries ID (e.g. 'LocID.*').", | ||
| ) |
There was a problem hiding this comment.
I thought I had started work on this in my PR?
One of the issues I was running into was that it needed to load the locations to be able to load the timeseries IDs.
I may have been focusing more on groups though and not individually specified TSIDs?
| "active": row["active_x"], | ||
| } | ||
| try: | ||
| result = cwms.store_timeseries_identifier( |
There was a problem hiding this comment.
I mentioned needing the locations above. But I wonder if using the store_timeseries_identifier works differently then me doing a store_timeseries directly?
If so we'd want to go with your method! I did not realize the store_timeseries worked without at least a base location present. (even if the type is different)
There was a problem hiding this comment.
no you have to do the location first. that is why I have a check to only load timeseries that have a location in the target database.
There was a problem hiding this comment.
Ah that must be the method you use for idenitfier. I do get 404 when I try with store_timeseries
you would need to update the json before loading since that would also have the office in it. I didn't see that being update so essentially the target office was not being used. I am not sure the use case either. the point is bulk loads from one database to anther not once office to another. |
|
That makes sense, im with you You might remove source office too and just use existing office for both? Or do you want to distinguish the office for loader purposes? I.e. something less likely to default from env? If so might remove the env option altogether for the loader office? |
2 participants
reorganize in load functions