-
Notifications
You must be signed in to change notification settings - Fork 616
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
AIO-interface: add Unhealthy container state #5307
base: main
Are you sure you want to change the base?
Conversation
docjyJ
commented
Sep 21, 2024
- Add Unhealty state
- Replace class by backed enum
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Jean-Yves, Simon asked me to have a look at your changes. I left some feedback regarding the code itself.
I really like you refactoring the container state logic from an interface to an enum. However, I think that both concerns could have been split into separate PRs. One PR to refactor the interface and another one to implement the new healthy states. But the code is already there so why not.
Looks good otherwise but did not test.
Hi, Thanks for the answer, as soon as I have time I'll look into it. I will try to separate and make several PR. Take care, |
I keep this PR open for the unhealthy state. |
1f3f231
to
b9ca83a
Compare
Conflicts :/ |
Don't worry, i'll handle it. |
b9ca83a
to
6a3c340
Compare
Solved and up to date and ready (to be tested anyway...) |
I have podman on my machine and I can't launch the container... I can't test it and I don't have time to debug... |
Why may I ask? Do you run into an issue here? |
podman run \
--rm \
--name nextcloud-aio-mastercontainer \
--volume nextcloud_aio_mastercontainer:/mnt/docker-aio-config:z \
--volume /var/run/user/1000/podman/podman.sock:/var/run/docker.sock:z \
--env WATCHTOWER_DOCKER_SOCKET_PATH=/var/run/user/1000/podman/podman.sock \
nextcloud/all-in-one:latest
I don't know what happen |
Maybe this helps? https://github.com/nextcloud/all-in-one?tab=readme-ov-file#are-there-known-problems-when-selinux-is-enabled |
It doesn't work, I give up. According to the docker documentation: https://docs.docker.com/reference/api/engine/version/v1.45/#tag/Container/operation/ContainerInspect State - Health - Status
Adding this information would be good for detecting potential problems and for debugging. |
Another question Why not use
Could we use this indicator? |
Docker doc for meaning of state https://docs.docker.com/reference/cli/docker/container/ls/#status |
91ac040
to
b903cc3
Compare
A proposal in commit b903cc3. (I renamed the enums to match the getters name, it's more consistent but I can rollback) |
I overread this. Yes sounds good imo :) |
Signed-off-by: Jean-Yves <7360784+docjyJ@users.noreply.github.com>
Signed-off-by: Jean-Yves <7360784+docjyJ@users.noreply.github.com>
74d15c0
to
41031cb
Compare