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

Allow mixing links and inventory variables #3501

Closed
wants to merge 6 commits into from

Conversation

cinek810
Copy link

@cinek810 cinek810 commented Apr 8, 2022

Previous logic allowed either link or direct specification in molecule.yml, while it's very convenient to link to group_vars containing some common values and set other (specific to molecule test) as host_vars.

Fixes: #2759

@cinek810

This comment was marked as outdated.

Previous logic allowed either link or direct specification in
molecule.yml, while it's very convenient to link to group_vars
containing some common values and set other (specific to molecule test)
as host_vars.
@amarao
Copy link
Contributor

amarao commented Apr 8, 2022

Hello. It looks amazing and it fixing one of weak points of Molecule! I'm very excited to see it fixed.

Can you, please, add a few tests? It can quickly become messy (there is a lot of corner cases), and having some tests will helps to cover them all.

@cinek810
Copy link
Author

cinek810 commented Apr 8, 2022

Sure. Can you point me to an example commit I can follow preparing the tests?

@ssbarnea ssbarnea added the bug label Apr 12, 2022
@ssbarnea ssbarnea removed the request for review from webknjaz June 1, 2022 07:37
@ssbarnea ssbarnea requested review from a team as code owners June 1, 2022 07:37
@ssbarnea ssbarnea requested review from ziegenberg, ganeshrn and cidrblock and removed request for a team June 1, 2022 07:37
@ssbarnea
Copy link
Member

ssbarnea commented Jun 1, 2022

Can someone step in here to add some tests, without any tests I will likely close it.

@cinek810
Copy link
Author

cinek810 commented Jun 1, 2022

If someone can point me to a commit (or external link) where similar tests where added I'll work on that. I just don't have relevant experience.

@ssbarnea
Copy link
Member

ssbarnea commented Jun 1, 2022

Can you point me to a minimal scenario that was failing before this change and that is working after it? ... that would be a test by itself.

@ssbarnea
Copy link
Member

Closing this due to lack of updates.

@lifedanceman
Copy link

Hello! Are there any changes about the pull request?

@jub-ioki
Copy link

jub-ioki commented Jun 10, 2024

@ssbarnea , could we re-open this please?
As for a minimal case where molecule fails without this code, here is one...

provisioner:
  name: ansible
  inventory:
    links:
      group_vars: ../../../../group_vars/
    host_vars:
      molecule-host1:
        enable_thing: true
platforms:
  - name: molecule-host1

In this case molecule will silently ignore host host_vars and enable_thing will not be true on molecule-host1. This behavior is documented, the documentation has the following statement:

You can use either hosts/group_vars/host_vars sections of inventory OR links. If you use both, links will win.

But that statement is buried pretty deeply in the documention, and there is no runtime error or warning. We could add a warning for it, but better yet to allow both, which is exactly what this PR does.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot use "inventory.links.group_vars" together "inventory.host_vars"
6 participants