Skip to content
This repository has been archived by the owner on Oct 27, 2021. It is now read-only.

Use relative paths in LFH result messages #428

Merged
merged 4 commits into from
Feb 3, 2021
Merged

Use relative paths in LFH result messages #428

merged 4 commits into from
Feb 3, 2021

Conversation

ccorley
Copy link

@ccorley ccorley commented Feb 2, 2021

This PR contains a number of changes:

  1. Updated the data field of the result of a POST to the Orthanc server to include the relative location of the image as stored in Orthanc, e.g.: instances/de11d211-6fcb8f9e-509b1519-5acd5d8e-7d127482/preview

  2. Added an /instances Orthanc service route to Kong that goes directly to Orthanc (the first non-connect API in Kong). This allows a REST call to Kong to retrieve original image information stored in Orthanc (with the Orthanc username and password for basic auth).

  3. Consolidated the configure-kong.sh script under container-support/oci. The compose/configure-kong.sh sources ../oci/configure-kong.sh.

  4. Removed all references to external host ip. e.g. LFH_CONNECT_EXTERNAL_HOST_IP: "127.0.0.1" because all relative urls can now be evaluated against kong.

Tested with dev and server compose profiles on amd64 and oci script on s390x.

@ccorley ccorley self-assigned this Feb 2, 2021
@ccorley ccorley requested a review from dixonwhitmire February 2, 2021 22:37
[ "$(uname -s)" == "Darwin" ] && export LFH_KONG_LFHHOST="host.docker.internal"
export LFH_KONG_CONNECT_HOST="localhost"
[ "$(uname -s)" == "Darwin" ] && export LFH_KONG_CONNECT_HOST="host.docker.internal"
export LFH_KONG_ORTHANC_HOST=${LFH_KONG_CONNECT_HOST}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We now need to connect to two different containers from Kong.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we will always need the default compose config and kong we could set a "default" compose file

COMPOSE_FILE=docker-compose.yml:docker-compose.kong-migration.yml

# then do the case statement 
export COMPOSE_FILE=${COMPOSE_FILE}:<whatever>

That may not be much of an improvement though.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. It may be a wash in terms of complexity, though. We can always make that change later.

lfhmllp=${LFH_CONNECT_MLLP_PORT}
kongmllp=${LFH_KONG_MLLP_PORT}
connect_host=${LFH_KONG_CONNECT_HOST:-${LFH_CONNECT_SERVICE_NAME}}
orthanc_host=${LFH_KONG_ORTHANC_HOST:-${LFH_ORTHANC_SERVICE_NAME}}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LFH_CONNECT_SERVICE_NAME is used by the OCI script, while compose uses LFH_KONG_CONNECT_HOST. This single configure-kong.sh script handles both cases.

@@ -66,18 +66,6 @@ function wait_for_cmd() {
return 1
}

function add_http_route() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

[ "$(uname -s)" == "Darwin" ] && export LFH_KONG_LFHHOST="host.docker.internal"
export LFH_KONG_CONNECT_HOST="localhost"
[ "$(uname -s)" == "Darwin" ] && export LFH_KONG_CONNECT_HOST="host.docker.internal"
export LFH_KONG_ORTHANC_HOST=${LFH_KONG_CONNECT_HOST}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we will always need the default compose config and kong we could set a "default" compose file

COMPOSE_FILE=docker-compose.yml:docker-compose.kong-migration.yml

# then do the case statement 
export COMPOSE_FILE=${COMPOSE_FILE}:<whatever>

That may not be much of an improvement though.

@ccorley ccorley merged commit c39ed5c into master Feb 3, 2021
@ccorley ccorley deleted the relative-paths branch February 3, 2021 04:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants