Make CLI wait for init to complete #225
Conversation
This change also detaches on UP, and then follows the faros init log. Following that log ensures that the CLI waits for the completion of the init.
thomas-gerber
requested review from
cjwooo,
tovbinm and
ypc-faros
as code owners
start.sh
Outdated
| fi | ||
|
|
||
| if ((run_cli)); then | ||
| faros_init_exit=$(docker-compose ps faros-init --format json | grep -Eo '"ExitCode"[^,]*' | grep -Eo '[^:]*$') | ||
| if [ "$faros_init_exit" != 0 ]; then | ||
| printf "an error occured during startup, exiting... \n" |
There was a problem hiding this comment.
- "An"
- Let's note that the error was during the startup of the faros init process + advise what to look for
There was a problem hiding this comment.
just to be clear, they would already know what to look for because it is a few lines above in the faros init log that is followed
start.sh
Outdated
| @@ -60,12 +59,17 @@ main() { | |||
| if [[ $(uname -m 2> /dev/null) == 'arm64' ]]; then | |||
| # Use Metabase images built for Apple M1 | |||
| METABASE_IMAGE="farosai.docker.scarf.sh/farosai/metabase-m1" \ | |||
| docker-compose up --build --remove-orphans $detach | |||
| docker-compose up --build --remove-orphans --detach && docker-compose logs --follow faros-init | |||
There was a problem hiding this comment.
- What happens with logs from other containers?
- I think there is a better way of doing this. WDYT? @willmarks @cjwooo
There was a problem hiding this comment.
Those logs still exist; you can access them from Docker Desktop or docker-compose logs.
I like this approach because the current behavior is just to follow all logs at once and require you to keep the terminal open, which is not very friendly. Just following the Faros init is better IMO, and has the added benefit of returning once that container exists, which we can then use to go to the next command (CLI) without race conditions.
start.sh
Outdated
| fi | ||
|
|
||
| if ((run_cli)); then | ||
| faros_init_exit=$(docker-compose ps faros-init --format json | grep -Eo '"ExitCode"[^,]*' | grep -Eo '[^:]*$') |
There was a problem hiding this comment.
Maybe you can use the same approach as here: https://github.com/faros-ai/faros-community-edition/blob/main/.github/workflows/release.yml#L88
There was a problem hiding this comment.
That was a good idea, but since I still want to follow the faros init log, I cannot use wait after because that container has already exited. Hence ps and grep.
There was a problem hiding this comment.
It doesn't matter that the container has already exited. It'll just give you the exit code immediately.
There was a problem hiding this comment.
Btw I'm okay with either but would love to minimize the grep dark magic 😄
This will harmonize the container name scheme separator to '-'. This matters because Docker Desktop has a setting that automatically converts V1 calls to V2 calls, which can be problematic if we use commands relying on container names. We also switch to using `docker wait` instead of `ps` to get exit code.
start.sh
Outdated
| @@ -19,15 +19,14 @@ email_prompt() { | |||
| } | |||
|
|
|||
| function setDefaults() { | |||
There was a problem hiding this comment.
Lets delete this unused function
| @@ -55,17 +50,22 @@ main() { | |||
| # Ensure we're using the latest faros-init image | |||
| export FAROS_INIT_IMAGE=farosai.docker.scarf.sh/farosai/faros-ce-init:latest | |||
|
|
|||
| docker-compose pull faros-init | |||
There was a problem hiding this comment.
@ypc-faros can you confirm removing that is ok? I don't know why it was in in the first place.
There was a problem hiding this comment.
Sorry I missed this. We actually need it to make sure we pull the latest faros init image.
Description
This change also detaches on UP, and then follows the faros init log. Following that log ensures that the CLI waits for the completion of the init.
Note that purposefully use grep instead of
jqto avoid making the latter a pre-requisite to runstart.sh.Type of change
Checklist