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

Add api_find_and_modify module #93

Merged

Conversation

felixfontein
Copy link
Collaborator

SUMMARY

While working on configuring my new router with the api_modify module, I noticed that it is not really adequate for some operations, like renaming the default bridge. Basically what it does is delete the old bridge, and then create a new one. Unfortunately this (probably the delete) leaves the router in a state where you cannot contact the API anymore, whence you need to reset the config and are back with the default config. Instead of manually having to query the default bridge's .id value and then changing its name with the api module, or using the command module to rename it, I created another module, api_find_and_modify, which allows to modify entries that are identified by something else.

For example, I can tell the module to look for a bridge with name=bridge, and modify its value name to foo. If it finds such a bridge it will change its name, and if it doesn't find it, it won't rename anything. (It's possible to tell the module how many matches you expect so it will fail if the wrong number of matches is found.) This is IMO perfect for such tasks where you need to rename something without deleting and recreating it. (Which is the only approach that really works for a fully generic api_modify module.)

This PR includes some parts of #91 that I needed for the tests - they are a lot easier to write with the mechanisms I added in that PR, and these in turn depend on the _api_data.py module utils which isn't used by this new module, but only by api_info and api_modify from the other PR. This PR can be merged independently from #91 (after one of them is merged, the other needs to be rebased). I'm locally working with another branch that combines all PRs (this, #91, #63) so I wouldn't mind if one or two of them could get merged soon, that will make my life a lot easier ;-) I think this one is pretty polished already (the module is pretty simple anyway), and #63 is also looking good, so I guess one of #63 and this could get merged first (and the other second). (We might even want to release a new version before finishing #91.)

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

api_find_and_modify

@codecov
Copy link

codecov bot commented May 22, 2022

Codecov Report

Merging #93 (50c5e8a) into main (d57de11) will increase coverage by 1.10%.
The diff coverage is 88.73%.

❗ Current head 50c5e8a differs from pull request most recent head 8584bca. Consider uploading reports for the commit 8584bca to get more accurate results

@@            Coverage Diff             @@
##             main      #93      +/-   ##
==========================================
+ Coverage   82.58%   83.68%   +1.10%     
==========================================
  Files          21       25       +4     
  Lines        1981     2372     +391     
  Branches      321      407      +86     
==========================================
+ Hits         1636     1985     +349     
- Misses        264      305      +41     
- Partials       81       82       +1     
Impacted Files Coverage Δ
tests/unit/plugins/modules/fake_api.py 68.98% <48.88%> (-22.91%) ⬇️
plugins/modules/api_find_and_modify.py 91.42% <91.42%> (ø)
plugins/module_utils/_api_data.py 95.91% <95.91%> (ø)
tests/unit/plugins/module_utils/test__api_data.py 100.00% <100.00%> (ø)
...s/unit/plugins/modules/test_api_find_and_modify.py 100.00% <100.00%> (ø)
tests/unit/plugins/modules/test_api.py 100.00% <0.00%> (ø)
plugins/modules/api.py 78.04% <0.00%> (+0.06%) ⬆️

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 d57de11...8584bca. Read the comment docs.

@felixfontein
Copy link
Collaborator Author

ready_for_review

@NikolayDachev
Copy link
Collaborator

Let's check this one after #91 is merged

@felixfontein
Copy link
Collaborator Author

@NikolayDachev I think we should really do this one first, the other PR is still far from complete. This one on the other hand is done.

Copy link
Collaborator

@NikolayDachev NikolayDachev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look ok

@felixfontein felixfontein force-pushed the api_find_and_modify branch from 50c5e8a to 8584bca Compare May 23, 2022 15:53
@felixfontein
Copy link
Collaborator Author

I rebased this one too since there also were conflicts due to the extended query PR.

@felixfontein felixfontein merged commit ff66ba9 into ansible-collections:main May 24, 2022
@felixfontein felixfontein deleted the api_find_and_modify branch May 24, 2022 16:23
@felixfontein
Copy link
Collaborator Author

@NikolayDachev thanks for reviewing this!

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