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

Update routeros_facts #101

Merged
merged 32 commits into from
Aug 21, 2020
Merged

Conversation

adeptvin1
Copy link
Contributor

SUMMARY

I added variables to the "Default" function
ansible_net_architec:
ansible_net_uptime:
ansible_net_cpu_load:
And added the "Routing" function to collect data about BGP and OSPF.

ISSUE TYPE
  • Bugfix Pull Request
  • Docs Pull Request
  • Feature Pull Request
  • New Module Pull Request
COMPONENT NAME

routeros_facts.py

ADDITIONAL INFORMATION

Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

You need a changelog fragment.

@felixfontein
Copy link
Collaborator

CC @heuels

Copy link
Contributor

@heuels heuels left a comment

Choose a reason for hiding this comment

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

Thanks for submitting a pull request! I've noted a few things that need correcting, but otherwise it's very good, thanks.

You'd also need to add unit tests and a changelog fragment.

plugins/modules/network/routeros/routeros_facts.py Outdated Show resolved Hide resolved
plugins/modules/network/routeros/routeros_facts.py Outdated Show resolved Hide resolved
plugins/modules/network/routeros/routeros_facts.py Outdated Show resolved Hide resolved
plugins/modules/network/routeros/routeros_facts.py Outdated Show resolved Hide resolved
plugins/modules/network/routeros/routeros_facts.py Outdated Show resolved Hide resolved
plugins/modules/network/routeros/routeros_facts.py Outdated Show resolved Hide resolved
plugins/modules/network/routeros/routeros_facts.py Outdated Show resolved Hide resolved
plugins/modules/network/routeros/routeros_facts.py Outdated Show resolved Hide resolved
plugins/modules/network/routeros/routeros_facts.py Outdated Show resolved Hide resolved
plugins/modules/network/routeros/routeros_facts.py Outdated Show resolved Hide resolved
@adeptvin1
Copy link
Contributor Author

Hello, I created changelog fragment according to the instructions, I hope that it turned out correctly.. Also, can you tell us in what format you need to attach the unit test? Can there be instructions for me to prepare it correctly? I don't have much idea about it yet.

@heuels
Copy link
Contributor

heuels commented Aug 15, 2020

There already is a unit test file for routeros_facts module, you should add your tests there. See this file for reference: https://github.com/ansible-collections/community.network/blob/main/tests/unit/plugins/modules/network/routeros/test_routeros_facts.py.

@@ -110,6 +123,34 @@
description: The list of neighbors from the remote device
returned: when interfaces is configured
type: dict

# routing
ansible_net_bgp_peer:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is indented wrongly. There is a leading space which shouldn't be there. (That makes it invalid YAML.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I corrected it.

@adeptvin1
Copy link
Contributor Author

While we are still doing this work I would like to thank you for your help, you are very cool guys. I hope that in time I will learn to be like you, thank you very much.

@adeptvin1
Copy link
Contributor Author

CI verification was successful! Unexpectedly for me, this required a lot of corrections, I am ready for the next step.

@felixfontein
Copy link
Collaborator

recheck

Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

LGTM. Also tested it with a real device, looks good!

@heuels if you want to check again feel free, otherwise let's merge this!

Copy link
Contributor

@heuels heuels left a comment

Choose a reason for hiding this comment

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

LGTM.

@adeptvin1, thank you so much for submitting this pull request!

@felixfontein felixfontein merged commit 6fa7295 into ansible-collections:main Aug 21, 2020
@felixfontein
Copy link
Collaborator

@adeptvin1 thanks a lot for contributing this!
@heuels thanks a lot for reviewing!

@felixfontein felixfontein added the needs_backport_to_stable_1 PR needs to be backported to the stable-1 branch label Aug 21, 2020
@felixfontein felixfontein added backport-stable-1 and removed needs_backport_to_stable_1 PR needs to be backported to the stable-1 branch labels Sep 17, 2020
patchback bot pushed a commit that referenced this pull request Sep 17, 2020
* Update routeros_facts.py

* Update routeros_facts.py

* Update plugins/modules/network/routeros/routeros_facts.py

Co-authored-by: Egor Zaitsev <[email protected]>

* Update plugins/modules/network/routeros/routeros_facts.py

Co-authored-by: Egor Zaitsev <[email protected]>

* Update plugins/modules/network/routeros/routeros_facts.py

Co-authored-by: Egor Zaitsev <[email protected]>

* Update plugins/modules/network/routeros/routeros_facts.py

Co-authored-by: Egor Zaitsev <[email protected]>

* Update plugins/modules/network/routeros/routeros_facts.py

Co-authored-by: Egor Zaitsev <[email protected]>

* Update plugins/modules/network/routeros/routeros_facts.py

Co-authored-by: Egor Zaitsev <[email protected]>

* Update plugins/modules/network/routeros/routeros_facts.py

Co-authored-by: Egor Zaitsev <[email protected]>

* Update plugins/modules/network/routeros/routeros_facts.py

Co-authored-by: Egor Zaitsev <[email protected]>

* Update plugins/modules/network/routeros/routeros_facts.py

Co-authored-by: Egor Zaitsev <[email protected]>

* Update plugins/modules/network/routeros/routeros_facts.py

Co-authored-by: Egor Zaitsev <[email protected]>

* Create 101_update_routeros_facts.yml

* Update plugins/modules/network/routeros/routeros_facts.py

Co-authored-by: Felix Fontein <[email protected]>

* Update changelogs/fragments/101_update_routeros_facts.yml

Co-authored-by: Felix Fontein <[email protected]>

* add test default function

* add test data

* Update routing_ospf_neighbor_print_detail_without-paging

* add test function "Routing"

* added information of version

* Removed extra spaces

* add one more version, "ansible_net_cpu_load"

* remove many blank lines

* remove 'too many blank lines'

* renamed arhitec to arch

* Update routeros_facts.py

* Update test_routeros_facts.py

* Update test_routeros_facts.py

* one more

* remove one route

* Update test_routeros_facts.py

* remove one neighbor

Co-authored-by: Egor Zaitsev <[email protected]>
Co-authored-by: Felix Fontein <[email protected]>
(cherry picked from commit 6fa7295)
felixfontein pushed a commit that referenced this pull request Sep 17, 2020
* Update routeros_facts.py

* Update routeros_facts.py

* Update plugins/modules/network/routeros/routeros_facts.py

Co-authored-by: Egor Zaitsev <[email protected]>

* Update plugins/modules/network/routeros/routeros_facts.py

Co-authored-by: Egor Zaitsev <[email protected]>

* Update plugins/modules/network/routeros/routeros_facts.py

Co-authored-by: Egor Zaitsev <[email protected]>

* Update plugins/modules/network/routeros/routeros_facts.py

Co-authored-by: Egor Zaitsev <[email protected]>

* Update plugins/modules/network/routeros/routeros_facts.py

Co-authored-by: Egor Zaitsev <[email protected]>

* Update plugins/modules/network/routeros/routeros_facts.py

Co-authored-by: Egor Zaitsev <[email protected]>

* Update plugins/modules/network/routeros/routeros_facts.py

Co-authored-by: Egor Zaitsev <[email protected]>

* Update plugins/modules/network/routeros/routeros_facts.py

Co-authored-by: Egor Zaitsev <[email protected]>

* Update plugins/modules/network/routeros/routeros_facts.py

Co-authored-by: Egor Zaitsev <[email protected]>

* Update plugins/modules/network/routeros/routeros_facts.py

Co-authored-by: Egor Zaitsev <[email protected]>

* Create 101_update_routeros_facts.yml

* Update plugins/modules/network/routeros/routeros_facts.py

Co-authored-by: Felix Fontein <[email protected]>

* Update changelogs/fragments/101_update_routeros_facts.yml

Co-authored-by: Felix Fontein <[email protected]>

* add test default function

* add test data

* Update routing_ospf_neighbor_print_detail_without-paging

* add test function "Routing"

* added information of version

* Removed extra spaces

* add one more version, "ansible_net_cpu_load"

* remove many blank lines

* remove 'too many blank lines'

* renamed arhitec to arch

* Update routeros_facts.py

* Update test_routeros_facts.py

* Update test_routeros_facts.py

* one more

* remove one route

* Update test_routeros_facts.py

* remove one neighbor

Co-authored-by: Egor Zaitsev <[email protected]>
Co-authored-by: Felix Fontein <[email protected]>
(cherry picked from commit 6fa7295)

Co-authored-by: adeptvin1 <[email protected]>
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.

3 participants