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

routeros: /export params depending on version #2435

Merged
merged 3 commits into from Dec 17, 2021
Merged

Conversation

azielke
Copy link
Contributor

@azielke azielke commented Dec 16, 2021

Pre-Request Checklist

  • Passes rubocop code analysis (try rubocop --auto-correct)
  • Tests added or adapted (try rake test)
  • Changes are reflected in the documentation
  • User-visible changes appended to CHANGELOG.md

Description

Fix Mikrotik Routeros Backups being empty for routeros versions <7.

Closes issue #2427

@jplitza
Copy link
Contributor

jplitza commented Dec 17, 2021

Tested successfully (without :remove_secrets) on ROS 6.48.6 and ROS 7.1.

@azielke Could you please fix the RuboCop styling warnings?

@codecov-commenter
Copy link

codecov-commenter commented Dec 17, 2021

Codecov Report

Merging #2435 (23466e3) into master (879d489) will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2435      +/-   ##
==========================================
+ Coverage   63.25%   63.28%   +0.02%     
==========================================
  Files          30       30              
  Lines        1497     1498       +1     
==========================================
+ Hits          947      948       +1     
  Misses        550      550              
Impacted Files Coverage Δ
lib/oxidized/node.rb 70.83% <0.00%> (+0.20%) ⬆️

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 879d489...23466e3. Read the comment docs.

vars(:remove_secret) ? '/export hide-sensitive' : '/export show-sensitive'
else
vars(:remove_secret) ? '/export hide-sensitive' : '/export'
end

This comment was marked as resolved.

@jplitza
Copy link
Contributor

jplitza commented Dec 17, 2021

Out of curiosity, what happens then no line matches installed-version? I have no clue about Ruby, but I guess that @ros_version = nil and @ros_version >= 7 throws a NoMethodError? Maybe the conditional should cover that case:

if not @ros_version.nil? and @ros_version >= 7

restore previous behaviour when routeros version could
not be found (run "/export" without parameters)
@azielke
Copy link
Contributor Author

azielke commented Dec 17, 2021

installed-version should always be present - but it would indeed throw NoMethodError (which would show an error when fetching the config). I think doing the check for nil would indeed be best as this would behave exactly the same as it did previously.

@jplitza
Copy link
Contributor

jplitza commented Dec 17, 2021

LGTM.

Since there's not mention of the newly added ROS 7 support in the changelog yet, it doesn't really seem mandatory to add an entry for this change - but the ROS 7 support should be added. Maybe a maintainer can do that outside of this PR

@mortzu mortzu merged commit 78a8003 into ytti:master Dec 17, 2021
@mortzu mortzu mentioned this pull request Dec 22, 2021
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.

None yet

4 participants