Closed mbcosta closed 1 month ago
Thanks @mbcosta to tackle this problem.
I think it's also time to get rid of not much used features and remove dead code.
_use_specific_user -> I guess it's not needed anymore
docky.main.service: this one is a bit tricky. The goal is to tell which container to open when docky run
1) simple -> we get rid of this, hardcode to odoo service and we are done.
Drawback: we loose other use case of docky, but do we have some ? We already have a strong dependency to gosu
2) pipe ->
docker compose config | grep docky.main.service
(and get rid of warning) or
docker compose config --format json
| jq "findthegoodfiler"
Drawback: more error prone, or add a dependency to jq but keep this functionnality
display_service_tooltip: may be replace it with traefik hostname
self.project.create_volume(): we can replace this with docker compose
CLI commands
dcpatched: one of the main features of docky. It allow us to docky open
without specifying a container id.
We can replace it with basic docker compose ps --status created | grep odoo
or lookup by label.
_get_services_info() can be removed;
create_volume can be done with docker-compose
docky init is gone by the way. test/voodoo -> lol !
About your commits, everything looks fine execpt the logging.
The goal was to specify the verbosity level from cmd line in set_log_level
BTW, @mbcosta if you have any trouble running Docky projects on Slackware meanwhile, you can always follow my little guide to run them using virtualenv (I switched my old computer to a Samsung Android tablet recently (using a Debian chroot in a rooted Termux), it works well but I didn't manage to compile a proper Docker enabled kernel for it (I have a simple Docker in an Alpine/qemu VM for simple things like wkhtmltopdf)). https://github.com/akretion/docky-odoo-template-shared/issues/2#issue-2373601960 Overall it works very well, but I'll still be happy to get a standard Docker one day...
Thank all for review
@hparfr I'm trying to make less changes as possible to make the review easy and smooth, as I'm not a mantainer, and I even create a new PR #158 to bump the Master branch from 7.10. for 8.0.0 and put some commits from this PR to reduce code to review, maybe after this merge we can start a clean up, what do you think?
About "we loose other use case of docky, but do we have some ?" and by reading the ISSUE https://github.com/akretion/docky/issues/155 I think one way to make Docky more abstract and usefful for other cases besides Odoo can be make possible change default command "docky run" by .env or compose.yml, so even for us if necessary each project can has a especific "docky run" or "docky logs" in the cases where some parameter need to be change or in the different stages of a project implementation, but this is just a speculation and should be debate in another PR or Issue.
In the last commit I make the "docky open" work as you explained, I have tested when a container start with "docky run" and "docky up" and just let the old code for comparison reason and after your tests and OK it can be removed.
Removed the commit of change Log, so back to use debug, but included "$ " before to simulate/show as a command if it's a problem I can see to remove
@rvalyi It's good to know other forms to run, what about the perfomance? Do you see any problems? By running things like the complete test of Brazilian Localization like CI it can take some time and by this reason I'm always look for improve the perfomance. After a while working with Docker and Docky now I have a better knowledge about this tools and be able to make some tests and contributions.
@clementmbr The error you report seems not related with this PR, as far I understand the Docker try to get a Folder pointed by Cache that was delete or moving. I saw similar messages when I looking to clean up docker because of "HD Overflow" and get messages like "No space left on device" to solve it I have tried
$ docker system prune
But didn't solve, the problem was in the folder /var/lib/docker and mostly /var/lib/docker/overlay2 , by check the folder
# du -lha /var/lib/docker/
99G /var/lib/docker/
In my case by saw some Folders with almost 1 year old I decided to delete, with the command bellow you be able to see cases with more than 300 days ( ref https://stackoverflow.com/questions/13868821/shell-script-to-delete-directories-older-than-n-days)
Check
$ find /var/lib/docker/overlay2 -type -d -ctime +300
Delete
$ find /var/lib/docker/overlay2 -type d -ctime +300 -exec rm -rf {} +
After did it start appear similar messages as yours, I think the reason was delete some overlays2 folder but there are still some references in the other folders, to solve it I runned the same command but in main folder
$ find /var/lib/docker/ -type d -ctime +300 -exec rm -rf {} +
Be careful, make backups if you have something important, you need to restart Odoo Service or reboot the machine after this, and if you don't have anything important and want to make a clean up
$ find /var/lib/docker/ -type d -ctime +1 -exec rm -rf {} +
Maybe another way to solve can be remove docker package and install again, the folder will be removed and recreate in the process, after this when you run "docky build" instead to look for Cache Folder will be create a new one.
EDIT: you ca also try to use the command $ docker compose build --no-cache
Hehe guess who is installing a Docky instance right now and may crash test the PR...
for the record I got the same mentioned: kwargs_from_env() got an unexpected keyword argument 'ssl_version'
error a couple of days ago. The ugly way I got it working was to edit the /home/test/.local/share/pipx/venvs/docky/lib64/python3.9/site-packages/compose/cli/docker_client.py file and changed:
kwargs = kwargs_from_env(environment=environment, ssl_version=tls_version)
to:
kwargs = kwargs_from_env(environment=environment)
Yes this is not the proper solution but it got the job done meanwhile. I'll look better at the PR later.
EDIT: so in fact to get it work I had to install Docky with pip and not pipx:
python3 -m pip install --user install git+https://github.com/akretion/docky.git@master --break-system-packages
Recent Ubuntu or Debian release will force you to use this --break-system-packages option, but as I used the --user flag, it won't actually mess with my host packages.
And then I edited/fixed the problematic file:
sudo vi /home/app/.local/lib/python3.12/site-packages/compose/cli/docker_client.py
easy peasy
After merge of https://github.com/akretion/docky/pull/158 was possible reduce the code to review here and solve the conflict file, now the main changes are direct related with adapt Docky to use Docker Compose version 2.
@rvalyi with the last commit "docky open" should be work as expect (I'm just waiting review to confirm if everything is right and after remove the old code), "docky pull" need to be test (but as a simple command it should be work without problem), so I think this PR can be use to avoid the necessity of patchs in other files.
@mbcosta thank you for the work. I'll try to create a basic CI workforce as I did for our boleto_cnab_api project and then things will be easier to review.
thanks @rvalyi will be better has a CI to check
@mbcosta @sebastienbeau
Thanks for the work.
docky open --root
does not work anymore with this version. (tested also with @sebastienbeau PR)
I do not believe it should stop us to release a new version though.
I use docky open and never docky open --root, I didn't even know it exists. In any case, when root access is required, docker exec and docker cp usually give enough options.
Still it should be an easy fix to restore this feature, please don't release a broken version
@florian-dacosta maybe you has tested with an old branch, the change made in _use_specific_user method solve this problem, I'm test with the branch of the @sebastienbeau and it's seems work as expected
@paradoxxxzero I agree to fixed the problem before release and it's seems fixed, as you can see in the image above
Yes there was a little problem in seb's branch and it's fixed now, thanks!
I have merged this work here : https://github.com/akretion/docky/pull/159
Update Docky for Docker Compose v2
Docker Compose V1 is deprecated
https://github.com/docker/compose
Warning
https://docs.docker.com/compose/migrate/
Some recently updates in the Python and PyYAML seems cause errors in Docky
If you try to reinstall Docky
PyYAML
To fix it's need to update from 'docker-compose' V1 in Python to 'docker compose' V2 in GO, by the change of the language is not more possible import docker compose direct by python, some projects try to reimplement:
Docker-Py https://github.com/docker/docker-py
Python on whales https://github.com/gabrieldemarmiesse/python-on-whales
Docker Composer V2 https://github.com/jensenkairos/docker-composer-v2
I'm choosed Python-on-whales
Some considerations
Here python-on-whales are just use for get Config files from project, it's possible use some of the commands like docker.compose.run() and others https://gabrieldemarmiesse.github.io/python-on-whales/sub-commands/compose/ but I keep it as before by Plumbum run in bash '$ docker compose run ...'.
Included a new command 'docky system' to get some System Infos OS Type, Kernel, OS, Docker, Docker Compose, and Docky versions; just to make easy debug errors related to some OS or old or new libraries versions, it's just a resume of '$ docker info'. Tests was made with:
In the case you are testing with another OS check the versions of the main libraries, tests with other OSs are welcome, should be better extract this commit? For testing the change for v2 seems usefull for debug but if necessary I can extract.
The Docky commnands working are build run logs ps up down kill restart in both OS , command docky open return error, the problem seems related to file dcpatched.py ( I let this file commented in the __init to avoid error with import, need to check if still is necessary, any help are welcome ) and docky pull need to be test.
Integrated the PR https://github.com/akretion/docky/pull/151 , with this LOG show the command(s) execute, I'm add "$ " before to show as a command docker compose ps $ docker compose ps / Should be better keep without $ ?
It's been usefull see what the command will be execute for check errors but there are a debate in the PR about this change, if necessary I can extract this commit.
I'm also tried integrated the PR https://github.com/akretion/docky/pull/152 for use pyproject.toml, but return error when try to install $ pipx install path_of_file/ --force
Other simple changes was made to removed deprecated encoding, change in License Header from '2018' to '2018-TODAY', call method super() don't need parameters, change simple quotes ' for double quotes " in python files and in the message about missing compose_file.py change print for logger.error.
The tests was made with a generic project in v14 with Brazilian Localization, futher tests with complex compose.yml parameters are welcome and necessary for a better review.
There are a error with Traefik when you run 'docky build', the log message don't show the complete error as 'docker compose build'
Docker Compose show the complete erro with the fix
Should be better if Docky show the same message or included the suggestion to solve.
If necessary, as said before, some committs can be extract for others PRs for a better review.
cc @rvalyi @renatonlima