Skip to content

Change parsing of docker info in dev build #14120

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

Merged
merged 2 commits into from
Feb 12, 2024

Conversation

jjwatt
Copy link
Contributor

@jjwatt jjwatt commented Jun 13, 2023

This is a non-functional change. The way os_info is populated with docker info and grep 'Operating System' breaks on podman and likely in other places. This makes it work on both podman and docker, and it will continue to return the exact same strings everywhere else.

SUMMARY

Fix call in dev build to get os_info from docker info so that it works on podman.

ISSUE TYPE
  • Bug, Docs Fix or other nominal change
COMPONENT NAME
  • Other
AWX VERSION
devel
ADDITIONAL INFORMATION

@jjwatt jjwatt marked this pull request as draft June 13, 2023 19:26
@jjwatt jjwatt marked this pull request as ready for review June 13, 2023 19:36
@jjwatt
Copy link
Contributor Author

jjwatt commented Jun 13, 2023

Important: We should really figure out what this os_info thing is for. As best I can tell, it gets passed to docker-compose/containers as an "OS" environment variable. But, let me tell you something: This is what the value looks like, literally:

  • Operating System: Fedora Linux 38 (Workstation Edition)
  • Operating System: NixOS 22.05 (Quokka)

Those strings are going into os_info.stdout (and you have to remember that it has a stdout which is annoying, too). But, why? And why does it have to be pulled from docker? There are so many better places to get the OS info and maybe even better places in that output. How would strings like the above ever be useful? Who's parsing those?

@shanemcd
Copy link
Member

IIRC we only use this to detect when we're running on a Mac:

if [[ "$OS" == *"Docker Desktop"* ]]; then
export SDB_NOTIFY_HOST='docker.for.mac.host.internal'
else
export SDB_NOTIFY_HOST=$(ip route | head -n1 | awk '{print $3}')
fi

@jjwatt
Copy link
Contributor Author

jjwatt commented Jun 13, 2023

IIRC we only use this to detect when we're running on a Mac:

if [[ "$OS" == *"Docker Desktop"* ]]; then
export SDB_NOTIFY_HOST='docker.for.mac.host.internal'
else
export SDB_NOTIFY_HOST=$(ip route | head -n1 | awk '{print $3}')
fi

That seems like a very legitimate use case for passing in that string to the container(s)! Since we've identified how it's used, we can probably better name/use it in the "staging" part. Like, we could even make a bool of that string that we pass through. If we really want to case over an os string instead of just looking for a static substring, then we probably would not be using this $OS, os_info for that. So we'd either end up with multiple "OS" vars or we'd end up changing this then. We should probably make a bool for the docker desktop case. I'll try to do that in a follow-up PR after this one gets merged.

This is a non-functional change. The way os_info is populated with docker info
and grep 'Operating System' breaks on podman and likely in other places. This
makes it work on both podman and docker, and it will continue to return the
exact same strings everywhere else.
@dmzoneill dmzoneill merged commit 3ae7221 into ansible:devel Feb 12, 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.

3 participants