[adamboutcher]

Summary

This if statement will enable the fact to work if you are using podman with docker symlink/support as the json syntax is lowercase on podman but camel/title case on docker.

Related Issues (if any)

#982
Possibly this too #749

Provide a detailed description of all the changes present in this pull request.
Just an if statement in the factor code (docker.rb)

Checklist

• 🟢 Spec tests.
• 🟢 Acceptance tests.
• Manually verified. (For example puppet apply)

[CLAassistant]

All committers have signed the CLA.

[Henrik-Hansson]

Hi @torgeiou anything missing? I can try to help out in that case.

[sbernhard]

Tested with podman version 5.4.0 and it solves #982

[sbernhard]

Can someone of the maintainers, like @shubhamshinde360, @amitkarsale, @span786, @ghoneycutt, @bastelfreak merge this change?

[adamboutcher]

Can someone of the maintainers, like @shubhamshinde360, @amitkarsale, @span786, @ghoneycutt, @bastelfreak merge this change?

I don't think perforce cares... A number of their modules have some serious compatibility issues which would be easy to resolve as they have pending PRs and nothing have been done, so I suspect this will remain the same.

[jst-cyr]

@adamboutcher @sbernhard : Apologies on the delay in getting responses here. I'm going through all the open PRs right now and trying to get them prioritized for the team.

It does look like the CLA hasn't been signed which might have been the reason this was blocked for so long? If so, we might have to open a new PR from engineering with the changes to bypass that blocker. I'll add this to the priority list and when the engineering team is able to review we'll see what we can do to get this updated.

[adamboutcher]

It does look like the CLA hasn't been signed which might have been the reason this was blocked for so long?

I signed it 2 years ago...

[jst-cyr]

@adamboutcher : Indeed, and I can see the CLA message saying that everybody had signed it after you submitted the PR, so I'm not sure what that license check is talking about (unless somebody sneaked something in and hadn't signed the CLA).

[span786]

Thanks @adamboutcher, and sorry this took so long. The fix is still needed: lib/facter/docker.rb hasn't changed since 2023, so main still crashes the whole docker fact (#982, and #749 before it). The cause is Podman's Docker compatibility layer returning lowercase keys from network inspect (id instead of Id). Facts are pluginsynced to every node in the environment, so this hits any host with podman-docker installed, even ones that never use this module.

I've added the allowed-for-ci label so CI runs on this PR. A few suggestions:

1. Simplify (inline suggestion below).
2. Add a spec. spec/unit/lib/facter/docker_spec.rb already stubs network inspect from spec/fixtures/facts/docker_network_inspect_<name>. A fixture with Podman-shaped output (lowercase keys) plus one example asserting that the docker fact resolves would cover the regression.
3. Note the limitation. Under Podman the fact will now resolve, but docker.network.managed_interfaces will stay empty. It's built by matching Docker's br-<12-char id> bridge names, and Podman names bridges podman0, podman1, ... That's fine for this PR, but a CHANGELOG/README line or code comment saying so would stop people expecting it to work.

I'll approve once CI is green.

[span786]

This can handle both runtimes without the if/else. It also skips a network with no ID rather than raising. Since the only rescue is JSON::ParserError, a NoMethodError here takes down the whole docker fact:

Suggested change

	network_id = if docker['network'][network]['Id'].nil?
	docker['network'][network]['id'][0..11]
	else
	docker['network'][network]['Id'][0..11]
	end
	# Docker returns `Id`; Podman's Docker compatibility layer returns `id`
	network_id = (docker['network'][network]['Id'] || docker['network'][network]['id']).to_s[0..11]
	next if network_id.empty?

(The next matters: an empty network_id would make %r{#{network_id}} match every interface.)