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

Add support for rootless Docker #43

Merged
merged 9 commits into from
Mar 1, 2024
Merged

Conversation

piemonkey
Copy link
Contributor

@piemonkey piemonkey commented Aug 28, 2023

This now works by adding an option to eds to connect the container to a docker network. There's also an option to use the existing behaviour, for backwards compatibility, this is used by default if the network option isn't used. I've updated the readme to show the new usage.

Setting a hostname that refers to the host machine doesn't work on
rootless docker, so this change adds support for that configuration. It
also drops the requirement to publish services on the host machine in
order to access them from ember.
@nvdk
Copy link
Collaborator

nvdk commented Sep 22, 2023

Thanks for this @piemonkey !

@madnificent
Copy link
Owner

Thanks for this! We should verify whether other tools (like app-http-logger and mu-cli) work with rootless Docker. If they do not then advising it here or describing it as such will likely lead some users astray and we should then move it into a separate section with the appropriate notes.

@piemonkey
Copy link
Contributor Author

Thanks for this! We should verify whether other tools (like app-http-logger and mu-cli) work with rootless Docker. If they do not then advising it here or describing it as such will likely lead some users astray and we should then move it into a separate section with the appropriate notes.

I've done some very basic investigation with app-http-logger and it seems to work, though I don't know enough about it to know if there are any particularly problematic use-cases that should be checked.

I haven't checked for mu-cil, I've added it to my todo list. I do have a note on there to check about a script that failed for me when trying to write to /tmp, but I think that was from before I switched to rootless, which was actually more problematic as I needed sudo to run docker commands, which messed with all sorts of tools (I wasn't about to give a system capable of granting super-root access rights (sudo without the audit trail) to run without sudo though...).

@madnificent
Copy link
Owner

Jotting down my thoughts for this makes me realize that this feature warrants upgrading the documentation to something more easily usable by users. Further inspection of the code also shows there are more goodies in it than I had anticipated. I keep reworking this comment but I'll leave it here in its current state. Thanks for the improvements, looking forward to have this addition!

I doubt there are problems with the current stacks but I wrote down some topics we should test for being fairly certain. These are not blocking.

I feel it'd be best to keep this as a second-class citizen for now as we see what works and how we can best interface with such a world. The current docs have this implicit perhaps but weaving it in makes it a bit vague.

The main usage has been made a bit more complicated (but we can probably simplify again) and I think the initial examples should stay as simple as possible. I've seen users get confused in having to write --proxy=http://host/ instead of --proxy=http://localhost/ and don't see much value in having to type --add-host by default. There is a cognitive overload when starting it with docker-ember in the simpler current form. I suppose it would be best to have --add-host as the default for now and perhaps turn it off when --network is supplied. Documentation for joining a network with the rootless docker would then best be suited in a specific section. Being able to join a network directly is nice and I can imagine this leads to a slight shift in development mindset.

Your work on extending this and documenting this leads to multiple preferred use-cases. Not belonging to this commit, but perhaps something for the future, is to have some options in ~/.config/edi/settings which streamline defaults for a specific developer. Examples are easy to imagine: if there's a network, then join the identifier of that; if there's only one stack running then join that; and perhaps --proxy could be an alias for --proxy=http://host/ lowering initial user confusion.

Do you see it feasible to have -a as the default? Does it feel like the documentation could use an uplift? The latter could be in a separate PR because this already adds much value through the help!

testing rootless with respect to other tooling

app-http-logger:

If it manages to log HTTP messages (the more likely problem) and docker stats (the less likely problem) then it should just work. I don't think there are peculiar edge-cases there. Ping me if you'd like to verify this together.

mu-cli

This may be a bit more fiddling to be sure. I think this covers more cases and that may be more of a challenge. There are also possible future extensions though I don't expect that to lead to new conditions. I believe that if we can run template based scripts (like mu script dev-script in a recent mu-javascript-template, which I'd expect to work) and are able to run project scripts (like mu script migrations new sparql yolo, which I suppose will work) then it's probably safe? If I recall correctly mu-cli needs some updates in order to cope with containers not existing in the new docker compose command.

I think there was something failing with /tmp/ outside of this but I forgot what the details were, probably something regarding the download of available image versions.

@piemonkey
Copy link
Contributor Author

--add-host is the default, it should be fully backwards compatible.

Right now the API is a little awkward because --add-host is the default. The awkwardness being, if you're on rootless, so --add-host causes the command to fail, if you want to use eds without a proxy, you need to specify --network, with a valid network, just to turn off --add-host. I was wondering if adding either a --no-add-host option or better, spotting that no --proxy is passed, would be a good addition to this.

In my limited testing, http messages did get logged by http-logger, I'm not sure about docker stats. I'll test mu-cli and either post results here or update the readme.

@madnificent
Copy link
Owner

--no-add-host could be -A then?

Further exending the readme to help new features find their place
README.md Outdated
@@ -269,7 +327,14 @@ and you can provide interactive answers.
edl -u
```

*Note*: `edl` assumes `edi` is available on your PATH
> [NOTE]
Copy link
Contributor

Choose a reason for hiding this comment

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

If you write > [!NOTE] GitHub renders the block quote with a blue sidebar thingy and and a ℹ️-like icon. Adding the ! shouldn't impact legibility for people reading the plain markdown file.

Also works with > [!WARNING] and > [!IMPORTANT]:

Note

Highlights information that users should take into account, even when skimming.

Important

Crucial information necessary for users to succeed.

Warning

Critical content demanding immediate user attention due to potential risks.

See: https://github.com/orgs/community/discussions/16925

@sergiofenoll
Copy link
Contributor

A --no-add-host or -A would be greatly appreciated because sometimes you might want to proxy a local development frontend to a production server (e.g. to make small layout changes but have representative data to compare with).
So there's cases where you don't have a docker network to connect to (because the stack isn't running locally) and you don't want to add a host in the docker invocation of eds.

@erikap erikap merged commit 42b995d into madnificent:master Mar 1, 2024
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.

5 participants