Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add gateway runner with docker containers #146

Merged
merged 1 commit into from
Jan 11, 2024
Merged

Add gateway runner with docker containers #146

merged 1 commit into from
Jan 11, 2024

Conversation

ebyhr
Copy link
Member

@ebyhr ebyhr commented Jan 4, 2024

No description provided.

@cla-bot cla-bot bot added the cla-signed label Jan 4, 2024
@ebyhr ebyhr requested review from mosabua and wendigo January 4, 2024 09:33
mosabua
mosabua previously requested changes Jan 5, 2024
Copy link
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

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

I am not convinced we should get rid of the docker compose setup. The runner is great but only for Java developers...

wdyt @vishalya @willmostly @Chaho12 ?

@ebyhr
Copy link
Member Author

ebyhr commented Jan 5, 2024

The runner is pretty easy to use compared to the existing steps. I believe even non-Java developers can use it.
Note that the existing step requires maven build either.

@mosabua
Copy link
Member

mosabua commented Jan 5, 2024

Ok .. lets get rid of docker compose .. but lets add info on how to use the runner somewhere.

@ebyhr ebyhr force-pushed the ebi/query-runner branch from e1b15bb to e0b66a2 Compare January 5, 2024 22:48
@rdsarvar
Copy link
Contributor

rdsarvar commented Jan 6, 2024

The runner is pretty easy to use compared to the existing steps. I believe even non-Java developers can use it. Note that the existing step requires maven build either.

One drawback I can think of from this new approach is it's built more like a manual integration test. Modifications and state that might've been applied to containers (ex: postgres) that you want persisted will spin down alongside the application run.

An example workflow being: You spin up the backend DB and initialize, spin up the gateway, run some queries, modify how the gateway handles query history in some fashion, and then attempt to spin the gateway back up again. In the current setup the DB state would clear and you'd be required to either: re-do all the previous steps, change the code and mount your changes as an init script, update the backend container to mount it's data to a volume, etc.

I think that's the core gain from leveraging a compose file is that you get a bunch of functionality (+life cycle management) out of the box through a few YAML tags.

Note: The way I see it is once the gateway is dockerized, it'd be nice to provide a compose file which includes the latest Gateway image and allows for new coming users to hop into a README, run docker compose -f localdev/docker-compose.yml up -d and then they'd have a detached but connected running ecosystem where they can freely spin a gateway container up/down.

Feel free to ignore this if you'd like 😄 I'll be away from the internet for the next week or so and unfortunately won't be able to respond

@ebyhr
Copy link
Member Author

ebyhr commented Jan 6, 2024

We can simply provide a flag to reuse containers like TESTCONTAINERS_REUSE_ENABLE in Trino (#159). No need to redo all steps.

Copy link
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

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

Lets also make sure that any references to the updated sql files are correct.. the installation and quickstart guides might use and mention them. Please check.

@ebyhr
Copy link
Member Author

ebyhr commented Jan 11, 2024

Addressed comments.

Copy link
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

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

I think we should merge this and improve later. If we find people still want docker compose setups we can introduce them again later when we get a container going and work towards helm charts/operator or whatever ..

@ebyhr ebyhr merged commit 667e447 into main Jan 11, 2024
1 check passed
@ebyhr ebyhr deleted the ebi/query-runner branch January 11, 2024 23:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants