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_facts module #88

Merged
merged 12 commits into from
May 12, 2022
Merged

Conversation

felixfontein
Copy link
Collaborator

@felixfontein felixfontein commented May 1, 2022

SUMMARY

Similar to the facts module, but uses the API instead of SSH. Inspried by the problems in #86 and #62. I also moved some common code from the api module to a new module_utils and a new docs fragment.

This does not support the config information yet, I wasn't able to (quickly) figure out how to actually get them with the API. If anyone knows how to do that, I'd happily add it :)

The code of the module is very similar to the facts module. It can be simplified a lot, might do that later.

@heuels @NikolayDachev what do you think?

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

api_facts

@codecov
Copy link

codecov bot commented May 1, 2022

Codecov Report

Merging #88 (71ef356) into main (02ecc0c) will increase coverage by 0.08%.
The diff coverage is 81.00%.

@@            Coverage Diff             @@
##             main      #88      +/-   ##
==========================================
+ Coverage   82.39%   82.47%   +0.08%     
==========================================
  Files          16       21       +5     
  Lines        1545     1866     +321     
  Branches      247      297      +50     
==========================================
+ Hits         1273     1539     +266     
- Misses        218      252      +34     
- Partials       54       75      +21     
Impacted Files Coverage Δ
plugins/modules/facts.py 83.00% <ø> (ø)
plugins/module_utils/api.py 44.44% <44.44%> (ø)
plugins/modules/api_facts.py 77.52% <77.52%> (ø)
tests/unit/plugins/modules/fake_api.py 95.38% <95.38%> (ø)
tests/unit/plugins/modules/test_api_facts.py 97.26% <97.26%> (ø)
plugins/doc_fragments/api.py 100.00% <100.00%> (ø)
plugins/modules/api.py 78.65% <100.00%> (+7.67%) ⬆️
tests/unit/plugins/modules/test_api.py 100.00% <100.00%> (+1.47%) ⬆️
tests/unit/plugins/module_utils/test_quoting.py 100.00% <0.00%> (ø)

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 02ecc0c...71ef356. Read the comment docs.

@NikolayDachev
Copy link
Collaborator

I will check next week, I don't have a pshisical time to do that early

@felixfontein felixfontein changed the title [WIP] Add api_facts module Add api_facts module May 3, 2022
@felixfontein
Copy link
Collaborator Author

ready_for_review

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.

api - those are ok

  • doc_fragments
  • module_utils
  • module

api_facts.py - I never used facts ( the old one) before :) so I want to do some testing on api_facts, please give me some time to play with it .

testing - fake_api \m/ ... 👍

Also I'm not sure what you mean with "This does not support the config information yet" probably I can help ?

@felixfontein
Copy link
Collaborator Author

Also I'm not sure what you mean with "This does not support the config information yet" probably I can help ?

The facts module also returns the result of /export and /export verbose. I've tried into looking to get the api_facts to export that as well, but it turns out that while you can run the /export command (using https://librouteros.readthedocs.io/en/3.2.0/path.html#arbitrary-command), its output isn't returned. Which makes it pretty useless :) What you can do is run it and let it write its output to a file. But then you would have to retrieve the contents of that file - probably with scp? - and afterwards clean the file. (And pick a filename which doesn't already exist in the first place.) That's way too much complication for this module IMO.

If someone wants to get the output of /export they'd have to do it "manually" with the api module (and ansible.netcommon.net_get or something like that).

@NikolayDachev
Copy link
Collaborator

Also I'm not sure what you mean with "This does not support the config information yet" probably I can help ?

The facts module also returns the result of /export and /export verbose. I've tried into looking to get the api_facts to export that as well, but it turns out that while you can run the /export command (using https://librouteros.readthedocs.io/en/3.2.0/path.html#arbitrary-command), its output isn't returned. Which makes it pretty useless :) What you can do is run it and let it write its output to a file. But then you would have to retrieve the contents of that file - probably with scp? - and afterwards clean the file. (And pick a filename which doesn't already exist in the first place.) That's way too much complication for this module IMO.

If someone wants to get the output of /export they'd have to do it "manually" with the api module (and ansible.netcommon.net_get or something like that).

Yes /export not working, will think on it, I know I try the same before and there was a particular reason but I don't remember it. We have a option to use REST api + ansible.uri (not sure if have /export) but this will work only with ros7

@NikolayDachev
Copy link
Collaborator

I do some testing, l really like the results :)

At this stage I think can be approved except we want to try solve /export somehow ?

@felixfontein
Copy link
Collaborator Author

Since /export seems more complicated (and not like something we can easily solve very soon), let's merge it now without that, and maybe try to add it later. In the original facts module config wasn't included by default anyway, so users had to explicitly enable it.

If that's ok I'll merge.

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.

ready to be merge

@NikolayDachev
Copy link
Collaborator

sorry by mistake click close :(

@felixfontein felixfontein merged commit 3d80cce into ansible-collections:main May 12, 2022
@felixfontein felixfontein deleted the api-facts branch May 12, 2022 14:17
@felixfontein
Copy link
Collaborator Author

@NikolayDachev no problem, no damage done :) Thanks a lot for testing 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