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

Remove postgres in espresso e2e test #148

Merged
merged 1 commit into from
Jul 18, 2024
Merged

Remove postgres in espresso e2e test #148

merged 1 commit into from
Jul 18, 2024

Conversation

ImJeremyHe
Copy link
Member

@ImJeremyHe ImJeremyHe commented Jul 17, 2024

Since this PR EspressoSystems/espresso-sequencer#1707 has been merged and the espresso-dev-node doesn't require us to run a postgres anymore, so we can remove relevant config and make it cleaner.
And we also added a dev-node port for debugging according to EspressoSystems/espresso-sequencer#1709

Copy link

@Sneh1999 Sneh1999 left a comment

Choose a reason for hiding this comment

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

Code LGTM! One question just for my own understanding - you mentioned that dev node doesnt require postgres but isnt it running a script here?

@zacshowa
Copy link
Member

zacshowa commented Jul 17, 2024

I would like to expand on Sneh's comment and check my own understanding of docker.

It seems as if postgres was recently added into the launch script for the dev node. Further, the env variables used for configuration would have been coming from the .env and docker-compose file in this repo. From my current understanding, this would imply that the espresso-dev-node dockerfile would be looking for these env vars, not finding them, and then failing to start postgres.

If this isn't the case, is postgres use being handled entirely within the espresso-sequencer repo now? Does omitting the postgres setup information mean that somewhere in the pipeline we decide to not start postgres gracefully?

Copy link
Member

@zacshowa zacshowa left a comment

Choose a reason for hiding this comment

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

I left one comment to further my understanding of the lifecycle of building the docker image related to these changes. Other than that LGTM 🚢

@ImJeremyHe
Copy link
Member Author

ImJeremyHe commented Jul 18, 2024

Currently postgres is a necessity for running the espresso builder. But for the developers using the dev node, they should not pay any attention to the our internal issues. So we integrated the postgres into the image and as what @Sneh1999 said, we use a shell script to run the postgres inside the Docker image. postgres is still being used in the espresso-dev-node, but we don't have to ask the users/developers to run a postgres anymore, which may be confusing to them.

@zacshowa I think you can get the answer from this script. This script will be copied to the image. The entry of espresso-dev-node image is this script and you can see this script will set the envs and run the postgres before startting the binary.

@ImJeremyHe ImJeremyHe merged commit 2c7f9a4 into integration Jul 18, 2024
8 checks passed
@ImJeremyHe ImJeremyHe deleted the jh/e2e branch July 18, 2024 01:22
zacshowa pushed a commit that referenced this pull request Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants