21.7 fb module containerfor mhc #283
Conversation
…se21.7-SNAPSHOT # Conflicts: # onprc_ehr/resources/etls/HansenQueriesToFile.xml
labkey-jeckels
left a comment
labkey-jeckels
left a comment
There was a problem hiding this comment.
I left a note about the ETL XML file changes.
Thanks for the other edits. I will work on getting the test to set the new property so that it passes, and then merge the PR when we get successful runs on TeamCity.
| <transform id="step2"> | ||
| <description>Export Query to File Share</description> | ||
| <source queryName="RandData_AnimalGroups" schemaName="onprc_ehr"/> | ||
| <destination type="file" dir="hansenFiles" fileBaseName="AnimalGroups-${TransformRunId}-${Timestamp}" |
There was a problem hiding this comment.
The lack of a means that you won't be able to run this ETL. That's fine from the automated test perspective, but will of course be an issue in the real-world.
|
@labkey-jeckels @jonesgaohsu You know there's already a linked schema in ONPRC/EHR that shares these data, right? Is there a reason the ETL needs to add this extra configuration instead of using the already existing feed of MHC data? |
|
@bbimber Note that these PRs are using a linked schema (backed by a list) to fake up the geneticscore data schema for testing purposes, instead of setting up the assays and other dependencies for its queries. We wouldn't set up this linked schema on production. It's a fine question as to whether we'd benefit from using that linked schema for the new query or not. If we do, I could update the test so that it fakes up a linked schema named MHC_Data instead. |
|
Alright, if it's just testing then sure it doesnt matter a lot. I dont know any background on what all these queries or ETL are for, but this one appears to be querying the MHC folder directly: My point is that it would be better practice for any code to use the publicly exposed views through the existing linked schema, rather than query that container directly. geneticscore.mhc_data has most of the MHC data, but it's not necessarily 100%. Therefore it would be better practice to use the queries that are exposed for general consumption, which is MHC_data_unified. I understand that with the recent changes the latter is basically a simple view over MHC_Data, but it would be far more future proof to ensure all consumers use that query and nothing in the MHC folder itself. |
labkey-jeckels
left a comment
labkey-jeckels
left a comment
There was a problem hiding this comment.
I'm approving and will merge, primarily to get the tests passing again.
Gary, when you're back online after your move, please consider my comments on reintroducing a for the ETL and Ben's comments about using the other query instead of the mhc_data table directly.
Rationale
added MHC Container to Module.xml and then reset the from statement for Randal_MHC Data to use the correct subsistute string
Related Pull Requests
Changes