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

bin/docker-dev-shell: Teach DOCKER env variable #5073

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bb010g
Copy link

@bb010g bb010g commented Apr 30, 2022

DOCKER=podman bin/docker-dev-shell now works.

LOCAL_CODE_DIR eases running bin/docker-dev-shell with MSYS2 Bash.

@bb010g bb010g requested a review from a team as a code owner April 30, 2022 09:09
@jeffwidman
Copy link
Member

jeffwidman commented Sep 15, 2022

👋 Thanks for this and sorry for the delay reviewing. It would be nice to support podman etc.

However, I see three unrelated changes here:

  1. $DOCKER
  2. switch echo -> printf
  3. $LOCAL_CODE_DIR

Since they are unrelated, can you break them out into separate PR's? It's just easier to discuss them that way, as I had a couple of questions about items 2 and 3.

Note to myself: for 3, see also https://github.com/dependabot/dependabot-core/pull/4056/files#diff-6fe5c8444caeeec4882a7d5173b9497a104b0f419f1d7c26b4fc848aa7e40d66R76

@bb010g
Copy link
Author

bb010g commented Sep 15, 2022

echo "$msg" to printf '%s\n' "$msg" shouldn't result in different functionality under Bash (echo's behavior with regard to escape sequences isn't portable), except that when $msg starts with - it'll print as expected and not try to be parsed as an option. It's good shell script hygiene. (See Bash Pitfall #14.)

The introduction of $LOCAL_CODE_DIR to override $(pwd) is because MSYS2's Bash (which is an easy way to run this without WSL2) prints a path on pwd that Podman's native Windows build can't deal with. I'm guessing this is similar to what that other pull request ran into. I don't know of a way to solve this without giving the user the ability to directly override this path.

@jeffwidman
Copy link
Member

Thanks, I'm actually aware of most of ☝️... but they're still unrelated changes, so at the very least they should be separate commits, and by that time it's easier to review/discuss as separate PR's.

Additionally, from time to time we have to revert commits/PRs for various unexpected reasons, and it's a lot nicer if we can keep that revert scoped to a single logical change so that we don't have a side effect of rolling back other changes that are still fine to keep.

To be clear, I'm 👍 on all these changes so far as I can tell from a quick glance at this PR... so should be straightforward, but they do need to be broken up.

`DOCKER=podman bin/docker-dev-shell` now works.

`LOCAL_CODE_DIR` eases running `bin/docker-dev-shell` with MSYS2 Bash.
@jeffwidman
Copy link
Member

@bb010g Did you see my comment ☝️ ?

Any interest in moving this forward by splitting it up?

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.

2 participants