Broken handling in run.sh of persistence.json provided as base64-encoded string via ENV_TOOL_INPUT environment variable when running Docker fhir-persistence-schema on PostgreSQL bug

[holger-stenzhorn]

Describe the bug
When running Docker fhir-persistence-schema on PostgreSQL the handling in run.sh of persistence.json provided as base64-encoded string via ENV_TOOL_INPUT environment variable is broken.

To Reproduce
Broken handling in run.sh of persistence.json provided as base64-encoded ENV_TOOL_INPUT environment variable (This bug report is based on branch issue-1797a.)

Before actually going into the issue(s) in run.sh there is already an issue in the README found at https://hub.docker.com/r/ibmcom/ibm-fhir-schematool where it says

time docker run  --env ENV_TOOL_INPUT=$(cat persistence.json | base64) ibm-fhir-schematool:latest

Besides the unnecessary but unproblematic mention of time, the name of the Docker image is incorrect and also to make the base64 encoding work on Linux systems the option -w 0 needs to be provided to disable line wrapping. Hence on Linux it should read

docker run --env ENV_TOOL_INPUT=$(cat persistence.json | base64 -w 0) ibm/ibm-fhir-schematool:latest

and on Mac (simply)

docker run --env ENV_TOOL_INPUT=$(cat persistence.json | base64) ibm/ibm-fhir-schematool:latest

But now coming to the actual run.sh issue(s) which can be found in the function process_cmd_properties:

At the top of the function

if [ ! -z "${TOOL_INPUT_USED}" ]

tests (correctly) whether the persistence.json is provided as string in the environment variable TOOL_INPUT_USED.

Looking at the following Linux "branch" the line

if [ -f /opt/schematool/workarea/persistence.json ]

below tests if the file persistence.json exists at the given location but that file cannot exist at that point in time (if not put there manually before somehow e.g. via a Docker volume).

So this also logically contradicts the use of TOOL_INPUT_USED holding the content of persistence.json as a string and the line

echo "${TOOL_INPUT_USED}" | base64 -d > /opt/schematool/workarea/persistence.json || true

which only then creates that file.

(Sidenote regarding the Darwin "branch":

echo "${TOOL_INPUT_FILE}" | base64 --decode > /opt/schematool/workarea/persistence.json || true

is also buggy because TOOL_INPUT_FILE just holds the path to the (temporary) file and so it should read here too

echo "${TOOL_INPUT_USED}" | base64 --decode > /opt/schematool/workarea/persistence.json || true

)

In the next line

RC=$(cat "${TOOL_INPUT_FILE}" | wc -l )

the result will always be 0 as no content has been written to the file TOOL_INPUT_FILE but rather (in the line above) to /opt/schematool/workarea/persistence.json.

(Also an issue in the Darwin "branch": Here it reads

RC=$(cat "${TOOL_INPUT_USED}" | wc -l )

but it should not be TOOL_INPUT_USED but TOOL_INPUT_FILE - well - if the latter would really contain the persistence.json.)

Now there is the line

echo "${TOOL_INPUT_USED}" | /opt/schematool/jq -r '.' > /opt/schematool/workarea/persistence.json

which breaks for any base64-encoded string in TOOL_INPUT_USED and therefore should rather read

echo "${TOOL_INPUT_USED}" | /opt/schematool/jq -rR '@base64d' > /opt/schematool/workarea/persistence.json

But then: Why is there actually the line

echo "${TOOL_INPUT_USED}" | base64 --decode > /opt/schematool/workarea/persistence.json 

before ?

I am terribly sorry if my "analysis" of the code is wrong but if it is correct then it would be great if it would be fixed... :-)

Expected behavior
n/a

Additional context
n/a

[prb112]

I've updated the readme

[prb112]

The analysis of the code and purpose of the lines is a bit 50/50 - part was correct - the last bit on the decoding is not. if you have a specific commandline you are using, and can share it, please let me know.

[holger-stenzhorn]

In order to ease the testing I have created a very simple test fixture using Docker Compose.

For that my local folder now contains the following files:

• persistence.json

  {
      "persistence": [
          {
              "db": {
                  "host": "postgres",
                  "port": "5432",
                  "database": "fhir",
                  "user": "fhir",
                  "password": "change-password",
                  "type": "postgresql"
              },
              "schema": {
                  "fhir": "fhirdata"
              },
              "behavior": "onboard"
          }
      ]
  }
  

• docker-compose.yml

  version: "3.8"
  
  services:
    ibm-fhir-schematool:
      image: ibmcom/ibm-fhir-schematool:4.5.2
      volumes:
        - ./wait-for-it.sh:/opt/schematool/wait-for-it.sh
        - ./run.sh:/opt/schematool/run.sh
      depends_on:
        - postgres
      user: root
      environment:
        - ENV_TOOL_INPUT
      entrypoint: sh -c "
        chmod +x wait-for-it.sh run.sh;
        ./wait-for-it.sh postgres:5432 -- ./run.sh"
  
    postgres:
      image: postgres:13.1
      environment:
        - PGDATA=/data/postgres
        - POSTGRES_DB=fhir
        - POSTGRES_USER=fhir
        - POSTGRES_PASSWORD=change-password

• wait-for-it.sh from https://github.com/vishnubob/wait-for-it

• run.sh in the latest committed version from the branch issue-1797a

First I perform the test with a not base64-encoded persistence.json:

holger@homer:~$ docker-compose down
holger@homer:~$ ENV_TOOL_INPUT=$(cat persistence.json) docker-compose up

This works basically as expected except for the warning base64: invalid input and my other before reported issues:

ibm-fhir-schematool_1  | base64: invalid input
ibm-fhir-schematool_1  | The tool behavior being executed is onboard
ibm-fhir-schematool_1  | run.sh - [INFO]: 2020-12-06_13:13:47 - Starting the onboard behavior
ibm-fhir-schematool_1  | run.sh - [INFO]: 2020-12-06_13:13:48 - Activating Network Path Connection...
postgres_1             | 2020-12-06 13:13:48.112 UTC [73] LOG:  incomplete startup packet
ibm-fhir-schematool_1  | run.sh - [INFO]: 2020-12-06_13:13:48 - [NETWORK_PATH_CHECK] - SUCCESS - Connected to the database - postgres:5432
ibm-fhir-schematool_1  | run.sh - [INFO]: 2020-12-06_13:13:48 - creating the schema
ibm-fhir-schematool_1  | + tee out.log
ibm-fhir-schematool_1  | + /opt/java/openjdk/bin/java -jar /opt/schematool/fhir-persistence-schema-4.5.2-cli.jar --prop db.host=postgres --prop db.port=5432 --prop db.database=fhir --prop user=fhir --prop password=change-password --db-type postgresql --create-schema-oauth --create-schema-batch --create-schema-fhir fhirdata --pool-size 2
ibm-fhir-schematool_1  | 2020-12-06 13:13:49.462 00000001    INFO   com.ibm.fhir.schema.app.Main Opening connection to: jdbc:postgresql://postgres:5432/fhir
ibm-fhir-schematool_1  | 2020-12-06 13:13:49.866 00000001    INFO s.postgresql.PostgreSqlAdapter The schema 'FHIR_ADMIN' is created or already exists
ibm-fhir-schematool_1  | 2020-12-06 13:13:49.868 00000001    INFO s.postgresql.PostgreSqlAdapter The schema 'FHIRDATA' is created or already exists
ibm-fhir-schematool_1  | 2020-12-06 13:13:49.872 00000001    INFO s.postgresql.PostgreSqlAdapter The schema 'FHIR_OAUTH' is created or already exists
ibm-fhir-schematool_1  | 2020-12-06 13:13:49.875 00000001    INFO s.postgresql.PostgreSqlAdapter The schema 'FHIR_JBATCH' is created or already exists
ibm-fhir-schematool_1  | 2020-12-06 13:13:49.881 00000001    INFO   com.ibm.fhir.schema.app.Main Processing took:   0.458 s
ibm-fhir-schematool_1  | 2020-12-06 13:13:49.882 00000001    INFO   com.ibm.fhir.schema.app.Main SCHEMA CHANGE: OK
ibm-fhir-schematool_1  | + set +x
ibm-fhir-schematool_1  | run.sh - [INFO]: 2020-12-06_13:13:49 - done creating the schema
ibm-fhir-schematool_1  | + tee out.log
...

Now I perform the test with a base64-encoded persistence.json:

holger@homer:~$ docker-compose down
holger@homer:~$ ENV_TOOL_INPUT=$(cat persistence.json | base64 -w 0) docker-compose up

Well, this does not really work as expected and so all I get is this:

ibm-fhir-schematool_1  | parse error: Invalid numeric literal at line 2, column 0
ibm-fhir-schematool_1  | The tool behavior being executed is 
ibm-fhir-schematool_1  | run.sh - [INFO]: 2020-12-06_13:16:13 - invalid behavior called, dropping through - ''

But if I now change in run.sh the line

echo "${TOOL_INPUT_USED}" | /opt/schematool/jq -r '.' > /opt/schematool/workarea/persistence.json

to

echo "${TOOL_INPUT_USED}" | /opt/schematool/jq -rR '@base64d' > /opt/schematool/workarea/persistence.json

then I get again the expected result:

ibm-fhir-schematool_1  | The tool behavior being executed is onboard
ibm-fhir-schematool_1  | run.sh - [INFO]: 2020-12-06_13:21:54 - Starting the onboard behavior
ibm-fhir-schematool_1  | run.sh - [INFO]: 2020-12-06_13:21:54 - Activating Network Path Connection...
postgres_1             | 2020-12-06 13:21:54.953 UTC [75] LOG:  incomplete startup packet
ibm-fhir-schematool_1  | run.sh - [INFO]: 2020-12-06_13:21:54 - [NETWORK_PATH_CHECK] - SUCCESS - Connected to the database - postgres:5432
ibm-fhir-schematool_1  | run.sh - [INFO]: 2020-12-06_13:21:55 - creating the schema
ibm-fhir-schematool_1  | + tee out.log
ibm-fhir-schematool_1  | + /opt/java/openjdk/bin/java -jar /opt/schematool/fhir-persistence-schema-4.5.2-cli.jar --prop db.host=postgres --prop db.port=5432 --prop db.database=fhir --prop user=fhir --prop password=change-password --db-type postgresql --create-schema-oauth --create-schema-batch --create-schema-fhir fhirdata --pool-size 2
ibm-fhir-schematool_1  | 2020-12-06 13:21:56.283 00000001    INFO   com.ibm.fhir.schema.app.Main Opening connection to: jdbc:postgresql://postgres:5432/fhir
ibm-fhir-schematool_1  | 2020-12-06 13:21:56.698 00000001    INFO s.postgresql.PostgreSqlAdapter The schema 'FHIR_ADMIN' is created or already exists
ibm-fhir-schematool_1  | 2020-12-06 13:21:56.700 00000001    INFO s.postgresql.PostgreSqlAdapter The schema 'FHIRDATA' is created or already exists
ibm-fhir-schematool_1  | 2020-12-06 13:21:56.705 00000001    INFO s.postgresql.PostgreSqlAdapter The schema 'FHIR_OAUTH' is created or already exists
ibm-fhir-schematool_1  | 2020-12-06 13:21:56.707 00000001    INFO s.postgresql.PostgreSqlAdapter The schema 'FHIR_JBATCH' is created or already exists
ibm-fhir-schematool_1  | 2020-12-06 13:21:56.713 00000001    INFO   com.ibm.fhir.schema.app.Main Processing took:   0.471 s
ibm-fhir-schematool_1  | 2020-12-06 13:21:56.715 00000001    INFO   com.ibm.fhir.schema.app.Main SCHEMA CHANGE: OK
ibm-fhir-schematool_1  | + set +x
ibm-fhir-schematool_1  | run.sh - [INFO]: 2020-12-06_13:21:56 - done creating the schema
ibm-fhir-schematool_1  | + tee out.log
...

So I am again sorry but in the end I am still having problems in understanding your code in run.sh as it seems not to completely work - at least not for me, I am afraid...

[prb112]

ibm-fhir-schematool_1 | base64: invalid input <-- this as expected, I test the input. I don't intend on fixing that as it is working as designed.

The problem is I am checking for a cert base64 which is absent when you are calling it not as intended or designed. I'll make a slight change to accommodate your use case, however I cannot modify it too much for my usecase.

[holger-stenzhorn]

I am sorry yet again but I do not understand what you mean with "I am checking for a cert base64 " and "you are calling it not as intended or designed" there...

I am just doing exactly what is described at https://hub.docker.com/r/ibmcom/ibm-fhir-schematool for onboarding only wrapping the normal Docker run procedure in a Docker Compose so that I can quickly overwrite the run.sh for testing purposes - but besides that everything is plain vanilla version 4.5.2 of your Docker image and how to call it.

Therefore I could also not use Docker Compose but rather simply create the Docker image containing your latest run.sh and then do a regular Docker run - yet this would semantically be no different. ...but I can certainly do that to if you want.

...or do you mean something completely different with what you say.

BTW: Your latest run.sh still contains the comment # only run if the file exists but after your fix you (now correctly) check for exactly the opposite there... 😄

[prb112]

I'll clean it up. I'll clarify the use-case the use in the documentation, however you aren't using it exactly as intended. I'm going to once again, make a slight modification, and I will make that change.

[holger-stenzhorn]

Yes, it would be great if you could clarify the really intended use: I believed that the use case would be that you have an empty PostgreSQL database and the (fitting) persistence.json configuration file and then you would run your Docker image with that configuration file to set-up (onboard) the database so that it can be used by the FHIR server. ...or is that not the actually intended use?

[prb112]

That's the correct use case. The intended use of the variable is incorrect. I need to re-read your comments, and address in the future, as it doesn't match my use and tests in our offering.

[holger-stenzhorn]

Yes, it would be great if you re-read my comments... 😄

Well, I do not understand when you say "The intended use of the variable is incorrect." as I am just doing what is written at https://hub.docker.com/r/ibmcom/ibm-fhir-schematool.

On that page it says under Use:

The solution should set the variables as Environment Variables or mounted into the workarea folder as a volume which contains the persistence.json file.

So I obviously do not go for "mounted into the workarea folder" but for "set the variables as Environment Variables".

Now, under Environment Variables it states:

Name	Purpose
ENV_TOOL_INPUT	Encoded String, in most circumstances base64 encoded or well escaped text, of the json

...and then under Configuration file - persistence.json for Mac :

docker run  --env ENV_TOOL_INPUT=$(cat persistence.json | base64) ibmcom/ibm-fhir-schematool:latest

...and for Linux:

docker run  --env ENV_TOOL_INPUT=$(cat persistence.json | base64 -w 0) ibmcom/ibm-fhir-schematool:latest

Therefore I am following exactly what is written on that page and I am using the variable ENV_TOOL_INPUT exactly the way it is described there!

But if what is described on your page there is not the actually intended use of the variable can you please explain what the intended use of the variable really is?

[prb112]

You seem to be doing something slightly different, as I do not hit this error in our test case and our OpenShift/Kubernetes cluster where we do the steps as documented. I'll test exactly what you are doing when I can. Once again, I am very much attempting to accommodate what you are intending to do.

[holger-stenzhorn]

Well, thanks for your help but perhaps you are doing something slightly different as you are performing your tests on a OpenShift/Kubernetes cluster... 🙂

I am just performing my tests following your own documentation for Mac and Linux at https://hub.docker.com/r/ibmcom/ibm-fhir-schematool on macOS Big Sur 11.0.1 with Docker Engine 19.03.13 and Ubuntu Server LTS 20.04.1 with Docker Engine 19.03.5.

I mean there have been quite some (rather obvious) bugs even in the released versions of the Dockerized ibm-fhir-schematool (and its documentation). ...and therefore it seems that your "test case" was not really able to catch them either.

[prb112]

I have used it also in docker-compose and on my local machine with docker run.

[holger-stenzhorn]

Ok, I see! ...but then I re-ask my question from above:

But if what is described on your [documentation] page there is not the actually intended use of the variable can you please explain what the intended use of the variable really is?

Or to speed up things... Can you perhaps simply drop here the configuration files and the command lines which do work for you on your local machine?

[prb112]

Here are the primary files - https://github.com/IBM/FHIR/tree/master/fhir-install/src/main/docker/ibm-fhir-schematool/examples/postgres

I added this... https://github.com/IBM/FHIR/blob/master/fhir-install/src/main/docker/ibm-fhir-schematool/DEVELOPMENT.md

I didn't keep it updated as I'll further update when I can

[prb112]

I've added some tests and cleaned up the code. it works for your use and my use. I'm not sure when I regressed the code behavior, but I did, I've fixed it, and your use-case turned out to be my use-case (just behaves slightly differently)

[holger-stenzhorn]

(If you do not have time to read the whole rant then simply scroll down to the end of this comment for a properly working solution.)

---

Thank you very much for trying fixing this bug but I must admit that I am not really impressed by the way you handled this issue. I am sorry if it now sounds harsh but your comments and replies make me feel that you might not be working with the necessary professional diligence but perhaps rather acting somewhat haphazardly. So I have my slight doubts that it just pure coincidence that (only) yesterday the necessary test cases appear in the GitHub repository and that exactly then you discovered that our use cases are actually the same. ...but the issue is not 100% fixed - see below.

Well, when you say that you are not sure when you regressed the code behavior, then why don't you simply look at the Git history of run.sh and then all becomes 100% obvious: In your initial commit of that file on 12.11.2020 (228207d) things look like...

function process_cmd_properties {
    if [ ! -z "${TOOL_INPUT_USED}" ]
    then 
        OS_TYPE="$(uname -s)"
        case "${OS_TYPE}" in
            Linux*)     echo ${TOOL_INPUT_USED} |  base64 -d > ${TOOL_INPUT_FILE};;
            Darwin*)    echo ${TOOL_INPUT_USED} |  base64 --decode > ${TOOL_INPUT_FILE};;
            *)          exit -1;;
        esac
    fi

    # Take the commandline and dump it out to the temporary inputfile.
    echo -n "" > ${TOOL_INPUT_FILE}
    for TOOL_ARG in ${TOOL_ARGS}
    do 
        echo "${TOOL_ARG}" | sed 's|--||g' >> ${TOOL_INPUT_FILE}
    done
}

First of all you do not need the distinction there between Linuxand Darwin as you do not have to write base64 --decode for Darwin but base64 --d does the trick there too and avoids code duplication, so one echo ${TOOL_INPUT_USED} | base64 -d > ${TOOL_INPUT_FILE}; suffices completely for both. But you do not actually need the echo and simply do base64 -d > ${TOOL_INPUT_FILE} <<< ${TOOL_INPUT_USED}. Well, anyway - the trouble is that the script is killed if a not-base64-encoded string is passed in but more problematic that directly after you empty the file ${TOOL_INPUT_FILE} again by calling echo -n "" > ${TOOL_INPUT_FILE}. So things fail which can be detected directly even when performing only a simple testing...

So the changes in your next commit on 13.11.2020 (f554c12) definitely make sense except for the fact that you still empty the file ${TOOL_INPUT_FILE} at the end with echo -n "" > ${TOOL_INPUT_FILE} and so this still cannot work at all.

...
                set +o errexit
                set +o pipefail
                echo ${TOOL_INPUT_USED} |  base64 -d > ${TOOL_INPUT_FILE}
                RC=$?
                if [ "${RC}" != '0' ]
                then
                    echo ${TOOL_INPUT_USED} > ${TOOL_INPUT_FILE}
                fi
                set -o errexit
                set -o pipefail
...

---

Your commit on 13.11.2020 (c5b3af8) was the base for my initial bug report...

function process_cmd_properties {
    if [ ! -z "${TOOL_INPUT_USED}" ]
    then 
        OS_TYPE="$(uname -s)"
        case "${OS_TYPE}" in
            Linux*)
                # Since the pipe is used in a couple places where it can fail
                # we don't want it to kill this script, and have overriden (temporarily)
                # the fail on pipe and errexit, we'll control the exits.
                set +o errexit
                set +o pipefail
                echo "${TOOL_INPUT_USED}" | base64 -d > /opt/schematool/workarea/persistence.json || true
                RC=$(cat "${TOOL_INPUT_FILE}" | wc -l )
                if [ "${RC}" = "0" ]
                then
                    echo "${TOOL_INPUT_USED}" | /opt/schematool/jq -r '.' > /opt/schematool/workarea/persistence.json
                fi
                set -o errexit
                set -o pipefail
                ;;
            Darwin*)
                # Since the pipe is used in a couple places where it can fail
                # we don't want it to kill this script, and have overriden (temporarily)
                # the fail on pipe and errexit, we'll control the exits.
                set +o errexit
                set +o pipefail
                echo "${TOOL_INPUT_FILE}" | base64 --decode > /opt/schematool/workarea/persistence.json || true
                RC=$(cat "${TOOL_INPUT_USED}" | wc -l )
                if [ "${RC}" = "0" ]
                then
                    echo "${TOOL_INPUT_USED}" | /opt/schematool/jq -r '.' > /opt/schematool/workarea/persistence.json
                fi
                set -o errexit
                set -o pipefail
                ;;
            *)  
                exit 1
                ;;
        esac
    fi

    # Take the commandline and dump it out to the temporary inputfile.
    echo -n "" > ${TOOL_INPUT_FILE}
    for TOOL_ARG in ${TOOL_ARGS}
    do 
        echo "${TOOL_ARG}" | sed 's|--||g' >> ${TOOL_INPUT_FILE}
    done
}

So after looking closer again at the above Linux branch, the line RC=$(cat "${TOOL_INPUT_FILE}" | wc -l ) will always return 0 as the file ${TOOL_INPUT_FILE} is never filled with any content at that stage and so if [ "${RC}" = "0" ] always evaluates to true and hence echo "${TOOL_INPUT_USED}" | /opt/schematool/jq -r '.' > /opt/schematool/workarea/persistence.json is incorrectly always called even for base64-encoded input.

Actually I do not understand why you need to call echo "${TOOL_INPUT_USED}" | /opt/schematool/jq -r '.' > /opt/schematool/workarea/persistence.json as echo -n "${TOOL_INPUT_USED}" > /opt/schematool/workarea/persistence.json does work fine, i.e. why you need to remove the newlines?

In the Darwin branch the variables are incorrectl swapped and so echo "${TOOL_INPUT_FILE}" | base64 --decode > /opt/schematool/workarea/persistence.json || true should be echo "${TOOL_INPUT_USED}" | base64 --decode > /opt/schematool/workarea/persistence.json || true and RC=$(cat "${TOOL_INPUT_USED}" | wc -l ) should read RC=$(cat "${TOOL_INPUT_FILE}" | wc -l ). But as said above the two branches are not necessary as base64 -d works both on Linux and Mac.

---

Finally, your latest commit from today (e6dc655) things now work for base64-encoded JSON but you kill the functionality to use not base64-encoded JSON. When you write that there was an error handling, what are you referring to? In your code before the call to jq will simply fail if you add invalid JSON but there was not really any error handling and nothing that truly verified the JSON. ...and in the Darwin branch you still have the wrong variable in echo -n "${TOOL_INPUT_FILE}" | base64 --decode > /opt/schematool/workarea/persistence.json || true which should be ${TOOL_INPUT_USED}.

---

So to conclude all this lamenting here you have a working function process_cmd_properties for you to use which I have - of course 😁 - tested successfully with both base64-encoded and a regular/non-base64-encoded persistence.json and both on macOS and on Ubuntu:

function process_cmd_properties {
    if [ ! -z "${TOOL_INPUT_USED}" ]
    then
        OS_TYPE="$(uname -s)"
        case "${OS_TYPE}" in
            Linux*|Darwin*)
                # Since the pipe is used in a couple places where it can fail
                # we don't want it to kill this script, and have overriden (temporarily)
                # the fail on pipe and errexit, we'll control the exits.
                set +o errexit
                set +o pipefail
                local file=/opt/schematool/workarea/persistence.json
                if [ ! -f ${file} ]
                then
                    base64 -d > ${file} <<< ${TOOL_INPUT_USED}
                    if [ ! -s ${file} ]
                    then
                        echo ${TOOL_INPUT_USED} > ${file}
                    fi
                fi
                set -o errexit
                set -o pipefail
                ;;
            *)  
                exit 1
                ;;
        esac
    fi

    # Take the commandline and dump it out to the temporary inputfile.
    echo -n "" > ${TOOL_INPUT_FILE}
    for TOOL_ARG in ${TOOL_ARGS}
    do 
        echo "${TOOL_ARG}" | sed 's|--||g' >> ${TOOL_INPUT_FILE}
    done
}

[prb112]

I appreciate your passion and hard work and thought behind each one of these issues.

I tried to replicate your issues as close as possible and bring forward my internal test-cases (originally skipped as I hard coded the tests for my use-case).

I hope my latest commit addresses your concerns.

I am happy to continue the discussion.

[holger-stenzhorn]

Well, I am sorry but I am a bit at a loss as to what else I could possibly discuss here as this seems to be not so fruitful.

I mean, even in the latest version of run.sh that you just committed there is still a bug which I already pointed out two times in the above, i.e. in the line echo -n "${TOOL_INPUT_FILE}" | base64 --decode > /opt/schematool/workarea/persistence.json 2> /dev/null || true the ${TOOL_INPUT_FILE} has to be replaced by ${TOOL_INPUT_USED}.

Also you keep your duplicated code for Linux vs. Darwin without any need or actual usefulness and you keep "interesting" code like RC=$(cat /opt/schematool/workarea/persistence.json | wc -l ) if [ "${RC}" = "0" ] instead of using the natural if [ ! -s /opt/schematool/workarea/persistence.json ] and etc.

Therefore, your last commit did not really address my concerns at all or even calm them down. On the contrary, after this discussion I am deeply concerned that the code quality of the IBM FHIR server in general as well as its development/testing process/procedures are not optimal and flawed as well.

Is there are possibility to escalate this issue in order for it to be checked and solved on a higher/lead level?

[holger-stenzhorn]

The bugs you are producing and how are fixing those bugs is quite actually revealing and sheds quite some light on how obviously "well" you personally follow standard quality procedures when it comes to code quality and testing:

Looking at persistence-offboard-example.json and persistence-onboard-example.json, one can see that you simply copied them over from your DB2 examples.

So in your first pushed commit of both files you had "type": "postgres", which directly fails because it needs to be "type": "postgresql", - so obviously not testing before committing.

Then until today you had "port": "50000",in both files which again is als wrong as it is (by default) `"port": "5432",: Did that work with PostgreSQL or dir you commit this without testing before?

...but wait a minute, oooops, you performed the fix only in persistence-onboard-example.json but not in persistence-offboard-example.json: So it seems you have again performed a commit without testing.

Well, at least you have finally changed today the "user": "db2inst1", to something that works, namely "user": "postgres",: Yippie!

...but of course your first shot at setting that was not so correct as you had "user": "posstgres", there: I wonder if that worked with PostgreSQL or why did you commit that change?

...and while I am at it: For PostgreSQL the element tenant should be removed because that is specific to DB2, right?

So where is your (personal) quality control? Does nobody check what you do? The way you seem to do your work here is actually very scary to me...

[JohnTimm]

@holger-stenzhorn Thank you for the detailed information that you have provided on this issue. This is an open source project. It is free software. Support is provided on a best effort basis. As you can see by the activity in our repository, this issue is not the only thing that Paul is working on. Clearly there are some nuances to this particular code that will require dedicated focus beyond what we can currently provide. We do, however, accept PRs from external contributors. If you have something working that fixes the issues you are seeing, then please put it into a pull request.

[holger-stenzhorn]

@JohnTimm Thank you very much for chiming in here! I totally understand that this is an open source project and it is free software - yet it is obviously not a "toy project" but rather backed and financed by a rather big company who uses this software - correct me if I am wrong - in its commercial offering IBM Watson Health. So for me this is in the same spirit as, just to name one example, the open source KeyCloak from your subsidiary RedHat which is used in many mission-critical environments. Well, for us concretely, IBM FHIR Server is supposed to be feed with clinical data - so quite a delicate and valuable asset - and this is quite obviously the same use case for pretty much all users of your software. Hence, I am not asking for any support at all but rather for that the quality necessary for such a use is properly ensured. Now, when I see how your staff is performing in such a haphazardous fashion at this particular piece of code and you suggest that he is working in many other places within the codebase too, then how can I rest assured that his coding there is on a level appropriate for that use case? In this concrete case at hand, we are also not talking about nuances, i.e. matters of taste and style, but rather about the fact that the things produced by your staff did not work properly at all - and this in publicly released versions of the tool.

[JohnTimm]

@holger-stenzhorn Some parts of the codebase are significantly more mature than others. What you are seeing, in this particular instance, is an immature part of the codebase getting exercised and bugs (quite expectedly) are getting uncovered. What you are also seeing is a very busy member of our team attempting to respond and support your effort in using this immature code while juggling other work. All software will have bugs -- regardless the size of the company supporting it, whether it is a "toy project" or whether it is "mission-critical". The fact that you are uncovering issues is a good thing and will only help us to improve the quality of our codebase. Going back what I said in my earlier comment -- we encourage and appreciate all external contributions. Please feel free to submit a PR with what you have working. Regarding your comment about "released software", I will take this as an action to clearly identify less mature / more experimental capabilities that are included in our releases.

[holger-stenzhorn]

@JohnTimm Thank you very much again for your reply! Could you perhaps point me to a warning in the release notes, the tool's page on Docker Hub, or somewhere else which tells me as a (regular) user that the officially released version of this tool is in an immature state and should therefore not be used in production environments?

Well, it is of course an obvious truism that any kind of software - even that developed by large, established companies - contains bugs, but that "killer argument" is too often used as an excuse to distract from one's own concrete deficiencies. So have you actually looked at the specific commits of your staff? Most bugs found in his code are there not because the code is exorbitantly complicated or complex but only because of the necessary diligence seems to be missing: If he would simply read a bit more properly and pay a little more attention to the "small print" then he would not have to redo and fix things over and over again - and in the end that means more time and less stress. Makes sense? 😄

I will do a pull request about this particular change tomorrow. ...but I am already expecting some interesting, lengthy discussions from especially those who seem to have no time. 😏

[JohnTimm]

Could you perhaps point me to a warning in the release notes, the tool's page on Docker Hub, or somewhere else which tells me as a (regular) user that the officially released version of this tool is in an immature state and should therefore not be used in production environments?

We do not currently have such a warning. My earlier comment was intended to indicate that I will take an action to make sure to include such a warning in the future so that it is abundantly clear which capabilities are in the "more experimental / less mature" category.

[tbieste]

I confirmed that the README was updated based on the original comments, and verified that the usage of TOOL_INPUT_FILE vs TOOL_INPUT_USED in the process_cmd_properties function was updated as well.