Allow api module to fail

[felixfontein]

SUMMARY

Right now, the module will not fail when return_result is called with status=False, since it looks for status == 'False' (string instead of boolean).

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

api

[codecov]

Codecov Report

Merging #39 (462a200) into main (3e4427b) will increase coverage by 0.56%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #39      +/-   ##
==========================================
+ Coverage   79.69%   80.26%   +0.56%     
==========================================
  Files          11       11              
  Lines        1197     1221      +24     
  Branches      163      162       -1     
==========================================
+ Hits          954      980      +26     
- Misses        181      183       +2     
+ Partials       62       58       -4     

Impacted Files	Coverage Δ
plugins/modules/api.py	71.50% <100.00%> (-0.93%)	⬇️
tests/unit/plugins/modules/test_api.py	98.52% <100.00%> (+2.39%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e4427b...462a200. Read the comment docs.

[felixfontein]

This took a bit longer, since there were some problems with the tests. These should be fixed now as well :)

[NikolayDachev]

will check today

[felixfontein]

(I rebased after merging #37 so the PR's branch contains that PR as well. I didn't make any changes to this PR's code.)

[felixfontein]

Ok, this should either be merged in a bit more than a hour, or it has to skip this community.routeros release. Otherwise we might miss Ansible 4.2.0.

[NikolayDachev]

Will try to check the test tonight

[NikolayDachev]

we can merge

[NikolayDachev]

closed by mistake

[felixfontein]

@heuels @NikolayDachev @jplitza I've updated the changelog fragment. Please take a look! Also if you think that this shouldn't just show up in a patch release, but potentially a minor or even a major release, please say so! This will very likely break people's roles and playbooks, when they rely on errors not resulting in task failures. We have to decide how breaking this is (all collections stick to semantic versioning).

Basically our options are:

• Put this in a patch release (1.2.1). We should only do this if we think this is an important bugfix that - even though it breaks people's workflows - should get out fast.
• Put this in a minor release (1.3.0). This is only useful if someone installs the collection manually (i.e. outside of the Ansible package) and uses a version restriction like < 1.2.0 to prevent new features.
• Put this in a major release (2.0.0). This indicates a breaking change. A consequence is that 2.0.0 will not be included in any Ansible 4.x.0 release, but only in Ansible 5.0.0 (to be released in mid November; see https://docs.ansible.com/ansible/devel/roadmap/COLLECTIONS_5.html).

What do you think?

[NikolayDachev]

@heuels @NikolayDachev @jplitza I've updated the changelog fragment. Please take a look! Also if you think that this shouldn't just show up in a patch release, but potentially a minor or even a major release, please say so! This will very likely break people's roles and playbooks, when they rely on errors not resulting in task failures. We have to decide how breaking this is (all collections stick to semantic versioning).

Basically our options are:

• Put this in a patch release (1.2.1). We should only do this if we think this is an important bugfix that - even though it breaks people's workflows - should get out fast.
• Put this in a minor release (1.3.0). This is only useful if someone installs the collection manually (i.e. outside of the Ansible package) and uses a version restriction like < 1.2.0 to prevent new features.
• Put this in a major release (2.0.0). This indicates a breaking change. A consequence is that 2.0.0 will not be included in any Ansible 4.x.0 release, but only in Ansible 5.0.0 (to be released in mid November; see https://docs.ansible.com/ansible/devel/roadmap/COLLECTIONS_5.html).

What do you think?

1.3.0 look ok will give a some time for playbook/roles fixes

[felixfontein]

1.3.0 will not mean that the release will happen later, it only changes the semantic of the release in terms of semantic versioning.

[jplitza]

This definitely isn't a patch level change. I'm even tempted to say this is worth a new major version, since (roughly) a third of the collection changes a significant portion of its API (namely the return value, if you will). November isn't so far away, and this change isn't really pressing (it's been this way like forever, hasn't it?)

[heuels]

For me it's also tempting to say it's a major-level release, but still I would stick with a minor-lever here.

[NikolayDachev]

1.3.0 will not mean that the release will happen later, it only changes the semantic of the release in terms of semantic versioning.

Yes I know, what I try to point here was -> " This is only useful if someone installs the collection manually (i.e. outside of the Ansible package) and uses a version restriction like < 1.2.0 to prevent new features."

Other question: Why we should care about "ansible base" release date (except something new is pushed from it which we should implement). If I remember correctly this was the idea of small collections .. to not wait for "ansible base" release .. ?

[felixfontein]

Other question: Why we should care about "ansible base" release date (except something new is pushed from it which we should implement). If I remember correctly this was the idea of small collections .. to not wait for "ansible base" release .. ?

The ansible-core release date does not matter at all. What potentially matters is the Ansible release date. Ansible is the community distribution "with batteries included", which consists of ansible-core and a set of collections, including community.routeros. Creating a 2.0.0 release now means that the community.routeros version included in Ansible 4.x.0 will be frozen, except if we backport things and create further community.routeros 1.x.y releases. While this is possible (and not that much work), backporting features is more complicated, so the community.routeros version included in Ansible 4.x.0 will probably only get bugfixes, but no more new features.

Considering that we aren't publishing much features anyway, that's probably ok though.

[NikolayDachev]

Well 1.3.0 ..

[felixfontein]

I've relabelled the patch as minor_changes in 1128009. I'll try to discuss this with some people in the Ansible community next week to get some more feedback; right now I'm tending a bit to make this a major release.

[NikolayDachev]

I've relabelled the patch as minor_changes in 1128009. I'll try to discuss this with some people in the Ansible community next week to get some more feedback; right now I'm tending a bit to make this a major release.

Any news?

[felixfontein]

From the feedback I got, this should really be a major release, since it is a breaking change in functionality. If that's fine with everyone, I'll adjust the changelog fragment accordingly.

[felixfontein]

@heuels @NikolayDachev is this ok for you? If yes, I'll create a stable-1 branch, merge this, and bump the next expected version in main to 2.0.0. (Then we can also think whether there are other breaking changes we want to do for a 2.0.0 release. Not sure if there is anything, but if there is something, that would be the time :) )

[NikolayDachev]

I'm OK with it, thanks a lot for the effort

[heuels]

Fine with me as well! Thanks for doing this.

[felixfontein]

@NikolayDachev @jplitza @heuels thanks for your reviews and feedback!