Skip to content
This repository has been archived by the owner on Jan 8, 2023. It is now read-only.

Update condition for missing network #119

Merged
merged 4 commits into from
Jul 20, 2022

Conversation

jdmarble
Copy link
Contributor

Network creation wasn't working for me until I changed this condition to look for the string 'network not found' instead of 'no such network'. I can't tell, but maybe this worked in the past and the message changed. It is currently defined here.

fixes #28

Network creation wasn't working for me until I changed this condition to look for the string `'network not found'` instead of `'no such network'`. I can't tell, but maybe this worked in the past and the message changed. It is currently defined [here](https://github.com/containers/common/blob/707829f80bc7623c6ecea000f2616977e178ccb5/libnetwork/types/define.go#L11).

fixes ansible-community#28
Copy link
Member

@ssbarnea ssbarnea left a comment

Choose a reason for hiding this comment

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

This is 100% incorrect because ansible does have an implicit AND between conditions and not OR. Please read and correct it to do an or between the two strings, so the conditional is not broken.

See https://docs.ansible.com/ansible/latest/user_guide/playbooks_conditionals.html

@ssbarnea ssbarnea added the bug This issue/PR relates to a bug. label Apr 18, 2022
@jdmarble
Copy link
Contributor Author

jdmarble commented Apr 18, 2022

This is 100% incorrect because ansible does have an implicit AND between conditions and not OR. Please read and correct it to do an or between the two strings, so the conditional is not broken.

See https://docs.ansible.com/ansible/latest/user_guide/playbooks_conditionals.html

I am familiar with the implicit AND, but I don't know why my change is incorrect. The logic used to look like this:

"If there is a message AND the message contains "no such network" (message that podman never outputs) AND the network information is not in the result..."

Since that message is not output by Podman under any circumstance (that I'm aware of), the network does not get created even when it should be. I didn't change the structure of the logic, I just corrected the text of the message being tested for:

"If there is a message AND the message contains "network not found" (message that podman outputs if the requested network does not exist) AND the network information is not in the result..."

Maybe my understanding of the situation is incorrect, but I don't think changing any of those ANDs to an OR would be correct.

I've tested this locally and it fixes the problem for me. Maybe the solution isn't universal across all versions of Podman? If I added a Molecule test would it be helpful?

@apatard
Copy link
Member

apatard commented Apr 19, 2022

fwiw:

  • podman 2.1.1 (found on sles15sp3) tells "network not found"
  • podman 3.0.1 (found on Debian bullseye) tells "no such network".

I know it's not directly related to this, but it's sad that the CI is missing a test for such kind of "basic" things. I was about to ask for adding it but I fear it's little bit too much so I won't do it...

@jdmarble
Copy link
Contributor Author

That's very helpful! I'll change it to something like this:

      when:
        - podman_network.results[0].msg is defined
        - "'network not found' in podman_network.results[0].msg or 'no such network' in podman_network.results[0].msg"
        - podman_network.results[0].networks is undefined

I don't think it'll be too difficult to add a test for this in the existing molecule scenario. I'll give it a shot this week. I'm not sure if I'll be able to figure out how to matrix in the different versions of podman.

@apatard
Copy link
Member

apatard commented Apr 21, 2022

I don't think it'll be too difficult to add a test for this in the existing molecule scenario. I'll give it a shot this week. I'm not sure if I'll be able to figure out how to matrix in the different versions of podman.

imho, it should be a new scenario and not a modification of default one so it's a little bit more work.

@ssbarnea ssbarnea requested review from a team as code owners July 19, 2022 11:21
@ssbarnea ssbarnea requested review from jseguillon and cidrblock July 19, 2022 11:21
Copy link

@jseguillon jseguillon left a comment

Choose a reason for hiding this comment

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

LGTM

@ssbarnea ssbarnea self-requested a review July 20, 2022 15:12
@ssbarnea ssbarnea dismissed their stale review July 20, 2022 15:12

My own

@ssbarnea ssbarnea enabled auto-merge (squash) July 20, 2022 15:14
@ssbarnea ssbarnea merged commit 649e3b6 into ansible-community:main Jul 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug This issue/PR relates to a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Starting pod with network fails when not creating the network manually
4 participants