Improvements to shell command

[joehoyle]

Currently we only forward $COLS and $ROWS to exec, but we should do the same on shell. Also, when you shell previosuly we were logging people in as www-data which means you can't do things like install new packages. I don't see any issue with using root and giving those caps on shell as it's likely you'll be doing stuff like that.

[rmccue]

I don't see any issue with using root and giving those caps on shell as it's likely you'll be doing stuff like that.

Will this cause wp-cli to complain about being root?

[roborourke]

If there's a user with sudo allowed that'd work. Won't affect composer server cli though

[rmccue]

Do the containers even have sudo? If so, that's definitely preferable to signing in as root

[joehoyle]

Ah yeah we don't have sudo, that's probably a better route.

[nathanielks]

sudo isn't available. The route I'd take would to leave the behavior as-is (signing in as www-data), but add an additional -u|--user flag which allows users to sign in as root. This mirrors the behavior of docker-compose exec and I think would be good for consistency.

[nathanielks]

Let's remove the -u root and instead make that an additional cli argument 🙏

[roborourke]

Do you want to finish this off @joehoyle or shall I?

[joehoyle]

Sorry @roborourke I haven't been able to get to this.

I still feel like a "normal" shell execution should let you install things etc. Passing -u might be idiomatic Docker, but Docker knowledge / expectations isn't really a pre-requisite for Local Server.

How does Chassis behave here? When you get a shell, to you have root permissions?

[roborourke]

No worries! Chassis/vagrant give you a vagrant user by default that has sudo capabilities

[rmccue]

Part of this is that Chassis is using a real full Ubuntu install, rather than a stripped back version, so it has sudoers/etc.

[nathanielks]

I still feel like a "normal" shell execution should let you install things etc.

I disagree. "Normal" users aren't privileged and require the use of sudo on non-docker systems, so there's a precedent for some sort of flag/adjustment to the command in order to make the command privileged (eg the use of sudo). In this case, I think it better to follow that pattern/precedent and require the user to add an additional argument to the command to say it should be privileged. It's better to assume unprivileged execution instead of making everything privileged.

Passing -u might be idiomatic Docker, but Docker knowledge / expectations isn't really a pre-requisite for Local Server.

Certainly not, but as I live on the command line and will speak for those who do, I would very much appreciate having flags being consistent across tools 🙂

[joehoyle]

Hmm in this case, it seems like having sudo might be a good in-between, and would be consistent with Chassis.

[nathanielks]

If the argument is to maintain consistency with Chassis, then we'd also need to change over from Alpine to Ubuntu. While I would be in favor of that eventually (Canonical has dramatically improved their Docker image), I don't think right now is the time to prioritize that. For right now, local-server users have to adjust how they install packages because they have to use apk instead of apt-get, so while we're asking users to change how they install packages, why not also ask them to follow a fairly standard practice in Docker by explicitly setting the root user when loading up a shell?

[roborourke]

Agree with @nathanielks there, trying to get meaningful parity with Chassis is going to be really difficult if not impossible. They're completely different environments just with the same services available.

I'm not opposed to signing in as root by default but it feels like the exception and not the rule to what you'd be doing on the container as a dev working on a site. Root use is more likely to be for debugging or working on the container itself. Typically I'd just be running CLI commands like wp shell. If I added/installed something to the container it wouldn't be available in production anyway.

I think it's simple enough to just document adding -- -u root to the command if you need root capabilities.

[joehoyle]

OK, I've removed the root from this PR, I think we should do a follow up, as I'd like to get the other fixes in this PR. Opened #112 for adding that.

Moved -u root to new ticket