ansible-community / ansible-vault

:key: Ansible role for Hashicorp Vault
BSD 2-Clause "Simplified" License
364 stars 194 forks source link

Molecule improvements: test supported platforms #308

Closed gardar closed 2 years ago

gardar commented 2 years ago
elcomtik commented 2 years ago

Nice work, I struggled with this in https://github.com/ansible-community/ansible-vault/pull/305 and it is already done here. I'll wait for the merge and rebase to continue with my work.

elcomtik commented 2 years ago

@gardar my personal opinion is that using action from robertdebock is better because you only define matrix and one generic molecule config. You may see that in my PR https://github.com/ansible-community/ansible-vault/pull/305.

Anyway thanks for fixing all of the lint issues :+1:

bbaassssiiee commented 2 years ago

Thanks!

gardar commented 2 years ago

@gardar my personal opinion is that using action from robertdebock is better because you only define matrix and one generic molecule config. You may see that in my PR #305.

Anyway thanks for fixing all of the lint issues +1

Well actually you can define the matrix once and having a single molecule config in either one, I don't think it has anything to do with the molecule github action you use. You can simply run molecule test --all' which cycles through all of the scenarios. Or if you just have the default scenario thenmolecule test` will suffice.

I personally think it's nicer to have multiple scenarios and then run each scenario in a separate job since then a single failure won't make your whole action fail, and you can simply re-run the failed job. It also means that you won't get all of the errors in a single log.

The scenarios only include configuration options that are relevant to that specific scenario, all of the default and shared options are located in the shared config under .config/molecule/molecule.yml. And you can actually overwrite any of those options by defining them in the scenario as you can see here: https://github.com/ansible-community/ansible-vault/blob/f2be6f990719820b92bca3f0571cd702bbb35382/.config/molecule/config.yml#L20-L21 https://github.com/ansible-community/ansible-vault/blob/f2be6f990719820b92bca3f0571cd702bbb35382/molecule/archlinux/molecule.yml#L11-L13

elcomtik commented 2 years ago

Well actually you can define the matrix once and having a single molecule config in either one, I don't think it has anything to do with the molecule github action you use.

I agree with you on this.

I personally think it's nicer to have multiple scenarios and then run each scenario in a separate job since then a single failure won't make your whole action fail, and you can simply re-run the failed job. It also means that you won't get all of the errors in a single log.

That's not the case, you may look at how would it look in this action run https://github.com/robertdebock/ansible-role-dns/actions/runs/2986880975/jobs/4788931073

The scenarios only include configuration options that are relevant to that specific scenario, all of the default and shared options are located in the shared config under .config/molecule/molecule.yml. And you can actually overwrite any of those options by defining them in the scenario as you can see here:

Currently, molecule scenarios are different only in 5 lines, specifically the names and images, which can be templated as you may see in https://github.com/robertdebock/ansible-role-dns/blob/master/molecule/default/molecule.yml

I have no intention to criticize or hate your work, just wanted to offer another solution to the same problem. I'm happy that tests are working now and we may further develop this wonderful role.

gardar commented 2 years ago

That's not the case, you may look at how would it look in this action run https://github.com/robertdebock/ansible-role-dns/actions/runs/2986880975/jobs/4788931073

Currently, molecule scenarios are different only in 5 lines, specifically the names and images, which can be templated as you may see in https://github.com/robertdebock/ansible-role-dns/blob/master/molecule/default/molecule.yml

Ah clever! I understand it now, he's statically defining his job matrix and dynamically generating new scenarios whereas I statically defined the scenarios and then dynamically generate the job matrix.

I see pros and cons with both approaches, his approach can be more minimalistic, especially when the platforms and scenarios are all similar, but I can see it getting problematic when there are a lot of differences. Also you'd need a way to feed in the values from the matrix when running the molecule test outside of the ci.

I have no intention to criticize or hate your work, just wanted to offer another solution to the same problem. I'm happy that tests are working now and we may further develop this wonderful role.

I didn't take it as a arguing or criticizing, it's good to have discussions and I'm always up to learning something new :smiley:

elcomtik commented 2 years ago

Yeah, there are pros and cons. I didn't considered running molecule tests from local machine due to my minimalisic dev environment.

I learned many new things also, so I'm happy with this discussion. We may let it go as current solution is good enough for this moment.

elcomtik commented 2 years ago

@gardar look at comment for https://github.com/ansible-community/ansible-vault/pull/308/commits/1f200ad3c04178eaa9de0d0c5374e135c2ff64d2, I'm not sure if you get notify.