-
Notifications
You must be signed in to change notification settings - Fork 47
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
Change ip vrf for router OS v7 #259
Conversation
Docs Build 📝Thank you for contribution!✨ This PR has been merged and the docs are now incorporated into |
plugins/module_utils/_api_data.py
Outdated
fields={ | ||
'comment': KeyInfo(can_disable=True, remove_value=''), | ||
'disabled': KeyInfo(default=False), | ||
'interfaces': KeyInfo(), | ||
'routing-mark': KeyInfo(), | ||
'name': KeyInfo(), | ||
'numbers': KeyInfo(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
numbers
does not seem to exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, if you are using the set
command it is there for choosing on which items you want to change attributes. This is for editing entries. Some of those options here are not actual attributes. The same for firewall rules even when using the add
command - you will have a place-before
parameter which is not a parameter of such a firewall rule but only for internal processing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation. I've made the change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution! Below a first comment from my side. Please note that you need to add a changelog fragment, but it might be easier to wait a bit until it's clear how this PR will look before adding it :)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #259 +/- ##
=======================================
Coverage 82.97% 82.97%
=======================================
Files 32 32
Lines 4046 4046
Branches 871 871
=======================================
Hits 3357 3357
Misses 506 506
Partials 183 183
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@felixfontein is it possible to have both versions for vrf path? So we can decide what path to chose depending on RouterOS version. |
@ialshaev yes, simply keep the old path and add a new one. Using versioning you should be able to say that the old one only works before version x.y, and the new one only with version x.y and newer. |
Added versioning. Pls review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks almost good now, see my comment below! Please don't forget the changelog fragment :)
plugins/modules/api_info.py
Outdated
@@ -136,6 +136,7 @@ | |||
- ip proxy | |||
- ip route | |||
- ip route vrf | |||
- ip vrf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be moved to the right position (lexicographically sorted), i.e. after ip upnp interfaces
. Same in the other file.
Done. Please review |
Co-authored-by: Felix Fontein <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
@liquorice-head thanks for implementing this! |
Always a pleasure ;) |
* Update api_info.py * Update api_modify.py * Update _api_data.py * Update _api_data.py * Update _api_data.py * Update api_info.py * Update api_modify.py * Update api_info.py * Update api_modify.py * Create 259-add-routeros7-support-for-ip-vrf.yml * Update changelogs/fragments/259-add-routeros7-support-for-ip-vrf.yml Co-authored-by: Felix Fontein <[email protected]> --------- Co-authored-by: Felix Fontein <[email protected]>
* Update api_info.py * Update api_modify.py * Update _api_data.py * Update _api_data.py * Update _api_data.py * Update api_info.py * Update api_modify.py * Update api_info.py * Update api_modify.py * Create 259-add-routeros7-support-for-ip-vrf.yml * Update changelogs/fragments/259-add-routeros7-support-for-ip-vrf.yml Co-authored-by: Felix Fontein <[email protected]> --------- Co-authored-by: Felix Fontein <[email protected]>
SUMMARY
ISSUE TYPE
COMPONENT NAME
ADDITIONAL INFORMATION