Skip to content

Conversation

@RemkoMolier
Copy link

…le for them

In the case wireguard is installed for only a subgroup of all the hosts in the play, the current solution will not work.

This is a quick fix to not create peers if the public key has not been set as a fact for this host.

…le for them

In the case wireguard is installed for only a subgroup of all the hosts in the play, the current solution will not work.

This is a quick fix to not create peers if the public key has not been set as a fact for this host.
@RemkoMolier
Copy link
Author

Hi @githubixx ,

would this be something you are willing to merge, or did I miss something that needs to be done in order for this to get attention?

Thanks in advance!

Remko Molier

@githubixx
Copy link
Owner

Hi @RemkoMolier , thanks for the PR. The problem with these "quick fixes" is that this often just means "works for me" 😉 But does it also work for others? Every time I merge such a request someone else is complaining that this or that doesn't work anymore 😉 The problem is how to test the change properly. The only possibility is with Molecule. But to cover every possible use case is impossible. The current default Molecule test I've implemented only covers my use case which is just a full VPN mesh of VMs of various different OSes. Pretty simple at the end.

So, normally I ask for a molecule.yml that "simulates" the use case. I know that most people can't run that Molecule test because the setup needed to run the test is not that simple to install (e.g. because KVM/Qemu is needed). But it should be possible to use the default Molecule test as a template and adjust accordingly and add a new test that covers the use case of the PR. Normally it's good enough to have an adjusted molecule.yml in the PR (maybe with only two or three Ubuntu 24.04 VMs if that is good enough). It doesn't need to work. But esp. provisioner.inventory.host_vars needs to populated with the values needed. Of course the VMs defined in platforms needs to be in the correct groups.

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