set environment variables needed for the focus blaze-and-sql endpoint to work when fhir2sql is enabled #246
Conversation
* add and set FOCUS_ENDPOINT_TYPE Co-authored-by: Jan <[email protected]> --------- Co-authored-by: davidmscholz <[email protected]> Co-authored-by: Jan <[email protected]>
Feat/ccp ppi
| ccp-patient-project-identificator: | ||
| image: samply/ccp-patient-project-identificator | ||
| container_name: bridgehead-ccp-patient-project-identificator | ||
| environment: | ||
| MAINZELLISTE_APIKEY: ${IDMANAGER_LOCAL_PATIENTLIST_APIKEY} | ||
| SITE_NAME: ${SITE_NAME} | ||
|
|
There was a problem hiding this comment.
Is this supposed to be in this PR?
There was a problem hiding this comment.
I would appreciate if this could make it into main with this PR. But we can move it to the next merge. It would add the PPI and it should not break anything.
There was a problem hiding this comment.
I would like it to be in a different PR for transparency reasons. However, we could merge them at the same-ish time.
There was a problem hiding this comment.
| ccp-patient-project-identificator: | |
| image: samply/ccp-patient-project-identificator | |
| container_name: bridgehead-ccp-patient-project-identificator | |
| environment: | |
| MAINZELLISTE_APIKEY: ${IDMANAGER_LOCAL_PATIENTLIST_APIKEY} | |
| SITE_NAME: ${SITE_NAME} |
agreed, move to different PR
ccp/modules/exporter-compose.yml
Outdated
| focus: | ||
| environment: | ||
| EXPORTER_URL: "http://exporter:8092" | ||
| AUTH_HEADER: "${EXPORTER_API_KEY}" |
There was a problem hiding this comment.
Is this set correctly? It says "header" in the key but the value only has an API key. focus docs state that there should be "ApiKey " in front
There was a problem hiding this comment.
@djuarezgf and I have agreed on the ApiKey type of the authentication some year ago. Is there a reason to have a different type?
There was a problem hiding this comment.
I'm not asking for a different auth type. The focus docs say there should be "ApiKey " in front but here that's not the case. Either focus docs or this PR are faulty.
There was a problem hiding this comment.
I looked into Focus' code: The Exporter uses a different header field (x-api-key) , so it is generally handled differently. This could be something to align in A) Explorter b) our other components using API-Keys. At the very least, this could and should be mentioned in Focus docs to be clear.
There was a problem hiding this comment.
@djuarezgf, @TKussel, and I had a discussion about it* because it's not the most proper way to do it. We agreed to use that header because it was urgent and didn't require changes in Exporter.
ETA: * in December last year
There was a problem hiding this comment.
@enola-dkfz thanks for jogging my memory. David chose the header recommended by OpenAPI, we chose the header recommended by the RFC and used throughout our components. Could you or @davidmscholz add a brief sentence to the focus documentation, that if the endpoint is exporter the semantics of the auth_header option changes?
There was a problem hiding this comment.
Oh that's complicated, because when the target app is Exporter, the endpoint type is actually Blaze (or Blaze and SQL). As you said, Focus has outgrown its architecture.
There was a problem hiding this comment.
Thanks for adressing that in samply/focus#185
| ccp-patient-project-identificator: | ||
| image: samply/ccp-patient-project-identificator | ||
| container_name: bridgehead-ccp-patient-project-identificator | ||
| environment: | ||
| MAINZELLISTE_APIKEY: ${IDMANAGER_LOCAL_PATIENTLIST_APIKEY} | ||
| SITE_NAME: ${SITE_NAME} | ||
|
|
There was a problem hiding this comment.
| ccp-patient-project-identificator: | |
| image: samply/ccp-patient-project-identificator | |
| container_name: bridgehead-ccp-patient-project-identificator | |
| environment: | |
| MAINZELLISTE_APIKEY: ${IDMANAGER_LOCAL_PATIENTLIST_APIKEY} | |
| SITE_NAME: ${SITE_NAME} |
agreed, move to different PR
No description provided.