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

Upd use docker compose v2 fixes #159

Merged
merged 6 commits into from
Oct 21, 2024

Conversation

sebastienbeau
Copy link
Member

@mbcosta I have done some additionnal fixes on your PR

We can merge it and then release a new version.

The next step is to add some test (I will try to do it in a separated PR)

if self.root:
user = "root"
return user
return self.project.get_user(service)

Choose a reason for hiding this comment

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

This breaks root right?

Copy link
Member Author

Choose a reason for hiding this comment

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

aahahha my finger didn't do what my brand was thinking !!
It's ok now

@@ -53,7 +52,7 @@ class DockyRun(DockyExec):
"""Start services and enter in your dev container"""

def _check_running(self):
if self.project.get_containers(service=self.service):
if docker.compose.ps(services=[self.service], all=True):
Copy link
Member

Choose a reason for hiding this comment

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

why all ?

Copy link
Member Author

Choose a reason for hiding this comment

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

if you do not pass all, you do not see the odoo container as it launch with "run". All is to show all container even if there are not launch with the normal way "docker compose up"

Copy link
Member

Choose a reason for hiding this comment

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

but will you get exited containers ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Note: in the previous version we had the same behavior

And normally you should never have exited container for the main container (as we launch it with --rm).

But if you have used manually docker compose run and you didn't have launch docker compose rm
then you have an exited container and docky run is blocked !

So we can first remove this existed container (as it will stay here indefinitely) and then start the container correctly. See commit : 0c60eeb

for _name, service in self.project.get("services").items():
docky_help = service.get("labels", {}).get("docky.help")
if docky_help:
logger.info(docky_help)

def create_volume(self):
"""Mkdir volumes if they don't exist yet.

Only apply to external volumes.
docker-compose up do not attemps to create it
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
docker-compose up do not attemps to create it
docker compose up create volumes automatically, but docker compose run don't

Copy link
Member Author

Choose a reason for hiding this comment

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

in fact the comment was totally wrong docker compose always create the volume but with the user root. I fix it

@sebastienbeau sebastienbeau force-pushed the UPD-use_docker_compose_v2-fixes branch from f219a8c to cfb645a Compare October 15, 2024 06:42
@sebastienbeau
Copy link
Member Author

@paradoxxxzero @hparfr updated

@mbcosta
Copy link
Member

mbcosta commented Oct 15, 2024

Thank you @sebastienbeau , if you think will be better has a "clean" commits I can see to include your changes of:

During this time I'm also think in a way to make Docky more abstract, what a thought was allow change the default command $ docky run by a parameter in the docker-compose.yml file, for example if include something like:

odoo:
environment:
DOCKY_RUN: "run -P --rm --use-aliases -e NOGOSU=True odoo gosu odoo bash"

or in the .env

odoo:
environment:
DOCKY_RUN: $DOCKY_RUN

This command will overridden the Default, with this we can allow each Project has, if necessary, a specific parameters Saved in the file and in the big projects the most part of the Developers can ignore those specific parameters by just typing docky run or the Docky can be use for projects beyond Odoo case, but it's just a test, a speculation, let me know if you think it can be useful or not.

@sebastienbeau
Copy link
Member Author

@mbcosta thanks for your feedback, I will squash the commit before merge them.

Regarding the option of docky run, I think we can do something better

  • we can make it customisable
  • we should used the option --user=root instead of requiring gosu
    I will propose something in a separated PR

@sebastienbeau
Copy link
Member Author

I have added a task for the option on docky run /exec #160

mbcosta and others added 5 commits October 21, 2024 09:44
- included library python-on-whales to get some Config information from compose.yml files.
- Remove the logic of getting ip information as now we always use traefik
- Raise the right error message when the config is not valid

Co-authored-by: Sébastien BEAU <sebastien.beau@akretion.com>
…cker info' for check errors related to OS or older or newer libraries versions.
Remove dcpatched and hack for kill as docker compose work correctly

Co-authored-by: Sébastien BEAU <sebastien.beau@akretion.com>
@sebastienbeau sebastienbeau force-pushed the UPD-use_docker_compose_v2-fixes branch from 0c60eeb to 9ced2c2 Compare October 21, 2024 07:57
@sebastienbeau sebastienbeau changed the base branch from UPD-use_docker_compose_v2 to master October 21, 2024 08:01
@sebastienbeau sebastienbeau merged commit 555bd16 into master Oct 21, 2024
@sebastienbeau sebastienbeau deleted the UPD-use_docker_compose_v2-fixes branch October 21, 2024 08:01
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.

4 participants