Hi sadi,

Thanks for this feedback! I’ll wait for this patch to be merged then, no biggies as it’s currently greetings season’s so no rush xD

I’ll probably have few patches after that especially around unifying options (filtering especially) on few modules.

Thanks for the answer!

Le lun. 26 déc. 2022 à 21:06, Sagi Shnaidman sshnaidm@redhat.com a écrit :

Hi, Gael,

Thanks for your contribution! Currently the tox-2.12 CI job always fails, it's because of tox version 4 changes. I add a workaround in the patch https://review.opendev.org/c/openstack/ansible-collections-openstack/+/86860... When it (or other solution) is merged, you're good to go with your patch. Sorry for the inconvenience .

Thanks

On Sat, Dec 24, 2022 at 4:22 PM Gaël THEROND gael.therond@bitswalk.com wrote:

...

Hi ansible collections openstack team!

I finally had time to list all my issues met with the project, created few bug reports and even contributed to a patch today (minor, mainly copy/paste) however I’ve few questions regarding the CI process!

Overall, what’s the rule with the CI code testing? I’ve read the contributing guide and had an eye on previous patches to see how it’s used but I’m having a hard time to find a real unified method. For instance, it seems that some module miss CI tasks (such as compute_service_info) or did I missed something?

Thanks a lot for all the good job!

-- Best regards Sagi Shnaidman

PS: tox issues have been fixed in master branch of Ansible OpenStack collection, please rebase your patches :)

On 27.12.22 11:40, Jakob Meng wrote:

Hello Gaël, thank you for giving us feedback on our Ansible modules and actually submitting a new module ☺️ We are currently lagging a bit in responses because we are trying to get release 2.0.0 of the Ansible OpenStack collection out of the door in January 2023.

As part of this effort we also refactored our CI integration tests, it is more consistent nowadays but still not complete. With compute_service_info you picked our worst case, it is tested in role nova_services 😬 A relict from the past.

Initially we planned to write one Ansible role per module, e.g. role project_info for openstack.cloud.project_info. But doing so produced a lot of redundant code. So during this year we changed our plan. Now we merge tests for *_info modules with their non-info equivalents. For example, tests for both modules federation_mapping and federation_mapping_info can be found in role federation_mapping.

Integration tests for volume_service_info would be located in Ansible role volume_service.

Instead of compute_service_info better take neutron_rbac_policies_info as an example of how to write and test *_info modules. Refactoring compute_service_info is still on my todo list 😅 Same goes for our docs on how to write modules etc. 🙈

Best, Jakob

On 27.12.22 00:11, Gaël THEROND wrote:

...

Hi sadi,

Thanks for this feedback! I’ll wait for this patch to be merged then, no biggies as it’s currently greetings season’s so no rush xD

I’ll probably have few patches after that especially around unifying options (filtering especially) on few modules.

Thanks for the answer!

Le lun. 26 déc. 2022 à 21:06, Sagi Shnaidman sshnaidm@redhat.com a écrit :

Hi,  Gael,

Thanks for your contribution! Currently the tox-2.12 CI job
always fails, it's because of tox version 4 changes. I add a
workaround in the patch
https://review.opendev.org/c/openstack/ansible-collections-openstack/+/868607

When it (or other solution) is merged, you're good to go with
your patch. Sorry for the inconvenience .

Thanks


On Sat, Dec 24, 2022 at 4:22 PM Gaël THEROND
<gael.therond@bitswalk.com> wrote:

    Hi ansible collections openstack team!

    I finally had time to list all my issues met with the
    project, created few bug reports and even contributed to a
    patch today (minor, mainly copy/paste) however I’ve few
    questions regarding the CI process!

    Overall, what’s the rule with the CI code testing?
    I’ve read the contributing guide and had an eye on previous
    patches to see how it’s used but I’m having a hard time to
    find a real unified method. For instance, it seems that some
    module miss CI tasks (such as compute_service_info) or did I
    missed something?

    Thanks a lot for all the good job!



-- 
Best regards
Sagi Shnaidman

On 30.12.22 23:14, Jeremy Stanley wrote:

On 2022-12-30 17:05:34 +0100 (+0100), Jakob Meng wrote:

...

PS: tox issues have been fixed in master branch of Ansible OpenStack collection, please rebase your patches :)

[...]

Zuul always tests by merging proposed changes to the latest state of the targeted branch, so rebasing should only be necessary in order to address merge conflicts. If you want to get updated test results which take the tox fixes into account, leaving a review comment like "recheck now that tox configuration is fixed" (or anything starting with the word "recheck" really) should be sufficient.

Without a rebase, a new (additional) merge commit will be created. Merge commits vs rebases is a controversial topic, both have their pros and cons [1][2]. For Ansible OpenStack collection, I prefer to rebase each patch, esp. before +w'ing it, to get a flat and readable history. This helps me with keeping track of what has to be backported from master branch to stable/1.0.0 branch. YMMV. 😉

[1] https://www.atlassian.com/git/articles/git-team-workflows-merge-or-rebase [2] https://stackoverflow.com/questions/457927/git-workflow-and-rebase-vs-merge-...