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

Improving docker tooling #435

Merged
merged 11 commits into from
Jan 18, 2022
Merged

Conversation

LupusMichaelis
Copy link
Contributor

Hi everyone,

While trying to set up and run the environment I encountered some little issues, so I fixed them and like to contribute them.

Please note that i used tabs in the Docker's entrypoint. It's not by spite for spaces, it's really because some bash constructs requires them (like heredocs). So if you want to revert that change, please be advise that it'll trigger strange bugs around the heredocs.
I fixed the Docker's entrypoint, its invocation and the commands to be more in line with what I learned to be container good practices. It's tricky, so if you need some explanation, I'm available.

I didn't want to break the current workflow of anyone, so I just added one feature that requires a .env file. I'd like to move there all configuration from the various environments, so they wouldn't be spread over the project and requires “complex” handling (just one file at the root of the project). Actually, settings are already collected through environments, so it could be pretty straight forward.

I removed container_name, as this might conflict when running many time the docker set up. I replaced it by aliases in network sections and using docker compose services for handling containers.

I added a service for the worker. I would need someone to take a look if the dependency's correct, as I don't know how this is working yet.

I passed the Dockerfile through Hadolint, but no output, no idea it's fine.

docker run --rm -i hadolint/hadolint < docker/Dockerfile

@pnu-s
Copy link
Member

pnu-s commented May 16, 2021

Hi @LupusMichaelis

Thanks for the PR and all the changes, although that will take some time to review 😄

Before talking about the changes themselves, I tried running it on my machine and got the following error:

$ docker-compose up

ERROR: Missing mandatory value for "user" option interpolating ${uid?'echo uid=$(id -u) >> .env'} in service "exodus-front": 'echo uid=$(id -u) >> .env'

@LupusMichaelis
Copy link
Contributor Author

It's fine to take your time, it's a substantial set of changes ;)

The error message isn't really an error, and I don't know how to make it clearer (that's why I talked about the .env file in my PR presentation). Actually, I made it so the image looks for a uid variable which contain your current user ID. Docker Compose will not use your shell environment for that, hence UID can not be used. To squash the error message:

echo uid=$(id -u) >> .env # I use id -u to because it's more compatible than $UID

That's part of the improvements I'd like to continue (moving all local development machine configuration specific into .env file). This would require a script or at least a mention in the documentation. I'm just surprise Docker Compose didn't mention the file's missing as it's part of the service definition.

@pnu-s
Copy link
Member

pnu-s commented Sep 18, 2021

@codeurimpulsif Did you have any chance to take a look at this proposed change?

(Sorry @LupusMichaelis, definitely took more time than I though 😨)

doc/install.md Outdated Show resolved Hide resolved
doc/install.md Outdated Show resolved Hide resolved
Copy link
Contributor

@codeurimpulsif codeurimpulsif left a comment

Choose a reason for hiding this comment

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

Thanks for this PR, it's a lot of cool improvements!
I think it just need to be documented for some things:

  • .env uid creation
  • Update exodus/exodus/settings/custom_docker.py example
  • Replace exodus by exodus-front or exodus-worker
  • Replace information message Exodus is ready. by Exodus DB is ready.

We can merge it soon if the documentation is updated.

@LupusMichaelis
Copy link
Contributor Author

Ok, I'll do the changes this week end.
Should I rebase on the main branch first?

@pnu-s
Copy link
Member

pnu-s commented Oct 1, 2021

Awesome, thanks @LupusMichaelis!
You can rebase now or after making the last chances, your choice :)

@pnu-s
Copy link
Member

pnu-s commented Dec 31, 2021

Hello @LupusMichaelis

Do you know by any chance if/when you will have the time to take a look at the last changes?
Would love to merge this :)

@LupusMichaelis
Copy link
Contributor Author

Ok, been sick then forget about it, I'll take care of this this week ^^

@LupusMichaelis
Copy link
Contributor Author

Ok, I did:

  • rebase on top of v1
  • update doc
  • ensure it's still working fine

doc/install.md Outdated Show resolved Hide resolved
@pnu-s
Copy link
Member

pnu-s commented Jan 18, 2022

Works perfectly, thanks again @LupusMichaelis (hope everything is ok regarding your health)

I just pushed a minor typo correction and I'll merge it now.

@pnu-s pnu-s merged commit 74a0524 into Exodus-Privacy:v1 Jan 18, 2022
@LupusMichaelis LupusMichaelis deleted the tooling/docker branch January 18, 2022 08:41
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.

None yet

3 participants