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

luci-app-dockerman: Add dockerd and docker-compose as dependency #6985

Merged
merged 1 commit into from
Oct 8, 2024

Conversation

tklengyel
Copy link
Contributor

When installing luci-app-dockerman the webui appears empty and unusable if these dependencies are not installed (see https://forum.openwrt.org/t/luci-app-dockerman-missing-configuration-on-23-05-02/184043). Fix it by declaring them.

CC @lisaac

@dannil
Copy link
Contributor

dannil commented Mar 12, 2024

Does luci-lib-docker also need dockerd and docker-compose as dependencies to work correctly? If that's the case it would probably make sense to add it to that Makefile instead since luci-app-dockerman depends on luci-lib-docker.

@systemcrash
Copy link
Contributor

Is openwrt updated to docker compose already, instead of the older docker-compose?

@tklengyel
Copy link
Contributor Author

Does luci-lib-docker also need dockerd and docker-compose as dependencies to work correctly? If that's the case it would probably make sense to add it to that Makefile instead since luci-app-dockerman depends on luci-lib-docker.

I don't think so, at least I can't see anything in luci-lib-docker to indicate that, it just seems to be a convenience library to provide access to the docker API.

@tklengyel
Copy link
Contributor Author

Is openwrt updated to docker compose already, instead of the older docker-compose?

Can you elaborate on what you mean? Is there a newer version of docker-compose with a different package name?

@systemcrash
Copy link
Contributor

Is openwrt updated to docker compose already, instead of the older docker-compose?

Can you elaborate on what you mean? Is there a newer version of docker-compose with a different package name?

https://github.com/docker/compose

@stokito
Copy link
Contributor

stokito commented Mar 12, 2024

Is there a newer version of docker-compose with a different package name?

The docker compose v2 changed command to docker compose but the package name remains docker-compose.

The PR shouldn't be merged.

The dockerman is a client for the Docker that uses REST API. It doesn't call the docker command and does not depends on it. Technically speaking the Docker host can be on another computer.

You still can install dockerd and your router but you don't need the docker command and you can use the dockerman instead and save space.

Instead we may show a message for a user to clarify this and propose to execute opkg install dockerd.

Ideally before any changes we need to sync the dockerman with upstream version from https://github.com/lisaac/luci-app-dockerman.
All changes needs to be copied and merged here and then it's author will fork it again but then send PRs to the repo
lisaac/luci-app-dockerman#157 (comment)

@systemcrash
Copy link
Contributor

Instead we may show a message for a user to clarify this and propose to execute opkg install dockerd.

See what I did in luci-app-910nd. There may be better ways of achieving it, but this works.

@tklengyel
Copy link
Contributor Author

Technically speaking the Docker host can be on another computer.

I think this would be a fairly niche setup. When I install luci-app-dockerman I expect to run dockerd on the router, especially when following the wiki OpenWrt as Docker container host

@systemcrash systemcrash marked this pull request as draft March 13, 2024 00:37
@feckert
Copy link
Member

feckert commented Mar 13, 2024

From my point of view, if I install luci-app-dockerman, then dockerd should run on the router and thus be installed as a dependency. The application luci-app-dockerman is therefore only intended as a little helper for the LuCI. This means that luci-app-dockerman should make it possible to quickly run a container on the router and monitor it. I would not do a complex setup via the LuCI with luci-app-dockerman but via docker-compose via the cli. So nothing big. If you need a more professional configuration and status interface, then we should use other applications, for example https://www.portainer.io.

@stokito
Copy link
Contributor

stokito commented Mar 13, 2024

especially when following the wiki OpenWrt as Docker container host

I fixed the Wiki, please review
https://openwrt.org/docs/guide-user/virtualization/docker_host?do=diff&rev2%5B0%5D=1710248402&rev2%5B1%5D=1710318155&difftype=sidebyside

@systemcrash
Copy link
Contributor

Hi @tklengyel any refresh on this? @feckert any other opinions? I think this is acceptable as a new dep. If no changes, we can merge.

@tklengyel
Copy link
Contributor Author

The fix on the wiki was a good enough solution to me but if you think it's good to merge this PR anyway I think that's a more robust solution.

@stokito
Copy link
Contributor

stokito commented May 1, 2024

If this covers 99% of usage and we don't have an other fix e.g. "Install docker" button then let's merge it. @tklengyel please move it from draft

@tklengyel tklengyel marked this pull request as ready for review May 1, 2024 13:20
@systemcrash
Copy link
Contributor

@tklengyel please do not include merge commits. Just do a rebase.

@tklengyel
Copy link
Contributor Author

@tklengyel please do not include merge commits. Just do a rebase.

Done. Figured I'll try out Github's update branch button not realizing that by default it does a merge 👎🏻

@sys-default
Copy link

In fact, this app is not quite suitable for this official v23.05 OpenWrt. Its situation is partly a result of the unique environment in mainland China, that most of developers and users are still using v21.03 OpenWrt with iptables Instead of nftables. As far as I know, the whole docker framework is also based on iptables rather than nftables, so it runs well in v21.03 OpenWrt. You may notice that lisaac, the developer of this app is a Chinese too.
OpenWrt v23.05 variants for mainland China users, such as immortalwrt, will add all the dependencies in their packages.
dockerman

@stokito
Copy link
Contributor

stokito commented Oct 8, 2024

@systemcrash could you merge the PR?

@sys-default the dockerd dependency problems is a separate topic. It should be possible to install it.

When installing luci-app-dockerman the webui appears empty and unusable if
these dependencies are not installed. Fix it by declaring them.

Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
@systemcrash systemcrash merged commit f827219 into openwrt:master Oct 8, 2024
3 checks passed
@tklengyel tklengyel deleted the fix branch October 9, 2024 11:34
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.

6 participants