Base for Yard docs #3303

skiera6 · 2024-10-26T22:26:36Z

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

Started yard documentation.
Changed namespaces in a way that will make for a better documentation.
Separated errors into classes.
Adapted rake tests.

Changed namespaces in a way that will make for a better documentation. Separated errors into class. Made the rake test work.

…s about the dsl in models)

lib/oxidized/model/aosw.rb

+          next if line =~ /Output \d Config/i
+          next if line =~ /(Tachometers|Temperatures|Voltages)/
+          next if line =~ /((Card|CPU) Temperature|Chassis Fan|VMON1[0-9])/
+          next if line =~ /[0-9]+\s+(RPMS?|m?V|C)/i


lib/oxidized/model/aricentiss.rb

+      cmd 'show system information' do |cfg|
+        cfg.sub! /^Device Up Time.*\n/, ''
+        cfg.delete! "\r"
+        comment(cfg).gsub(/ +$/, '')


lib/oxidized/model/aricentiss.rb

+        cfg.each_line.map do |l|
+          next '' if l =~ /^Building configuration/
+
+          comment_next = 2 if l =~ /^Switch ID.*Hardware Version.*Firmware Version/
+
+          if comment_next.positive?
+            comment_next -= 1
+            next comment(l)
+          end
+
+          l
+        end.join.gsub(/ +$/, '')


lib/oxidized/model/asa.rb

+        cfg.gsub! /^passwd (\S+) (.*)/, 'passwd <secret hidden> \2'
+        cfg.gsub! /username (\S+) password (\S+) (.*)/, 'username \1 password <secret hidden> \3'
+        cfg.gsub! /(ikev[12] ((remote|local)-authentication )?pre-shared-key) (\S+)/, '\1 <secret hidden>'
+        cfg.gsub! /^(aaa-server TACACS\+? \(\S+\) host[^\n]*\n(\s+[^\n]+\n)*\skey) \S+$/mi, '\1 <secret hidden>'


lib/oxidized/model/asa.rb

+        cfg.gsub! /username (\S+) password (\S+) (.*)/, 'username \1 password <secret hidden> \3'
+        cfg.gsub! /(ikev[12] ((remote|local)-authentication )?pre-shared-key) (\S+)/, '\1 <secret hidden>'
+        cfg.gsub! /^(aaa-server TACACS\+? \(\S+\) host[^\n]*\n(\s+[^\n]+\n)*\skey) \S+$/mi, '\1 <secret hidden>'
+        cfg.gsub! /^(aaa-server \S+ \(\S+\) host[^\n]*\n(\s+[^\n]+\n)*\s+key) \S+$/mi, '\1 <secret hidden>'


lib/oxidized/model/ios.rb

+        cfg.gsub! /^(\s+standby \d+ authentication) .{1,8}$/, '\\1 <secret hidden>'
+        cfg.gsub! /^(\s+standby \d+ authentication md5 key-string) .+?( timeout \d+)?$/, '\\1 <secret hidden> \\2'
+        cfg.gsub! /^(\s+key-string) .+/, '\\1 <secret hidden>'
+        cfg.gsub! /^((tacacs|radius) server [^\n]+\n(\s+[^\n]+\n)*\s+key) [^\n]+$/m, '\1 <secret hidden>'


lib/oxidized/model/quantaos.rb

+      # @!method prompt(regex)
+      #   Sets the prompt for the device.
+      #   @param regex [Regexp] The regular expression that matches the prompt.
+      prompt /^\((\w|\S)+\) (>|#)$/


lib/oxidized/model/quantaos.rb

+      cmd 'show run' do |cfg|
+        cfg.each_line.select do |line|
+          (not line.match /^!.*$/) &&
+            (not line.match /^\((\w|\S)+\) (>|#)$/) &&


lib/oxidized/model/sonicos.rb

+          next if line =~ /date \d{4}:\d{2}:\d{2}/
+          next if line =~ /time \d{2}:\d{2}:\d{2}/
+          next if line =~ /system-time "\d{2}\/\d{2}\/\d{4} \d{2}:\d{2}:\d{2}.\d+"/
+          next if line =~ /system-uptime "((\s+up\s+\d+\s+)|(\d+\s\w+(,\s)?)*)"/


lib/oxidized/model/sonicos.rb

+          next if line =~ /date \d{4}:\d{2}:\d{2}/
+          next if line =~ /time \d{2}:\d{2}:\d{2}/
+          next if line =~ /system-time "\d{2}\/\d{2}\/\d{4} \d{2}:\d{2}:\d{2}.\d+"/
+          next if line =~ /system-uptime "((\s+up\s+\d+\s+)|(\d+\s\w+(,\s)?)*)"/


skiera6 · 2024-11-14T15:37:22Z

This commit should fix couple of the rubocop errors, my IDE was acting up and instead of keeping the changes i wanted it merged both old and new code. Just to be safe i will review the pull request when i have some free time also i didn't want to change metrics and i won't change cnos model just to be safe that it won't break anything.

robertcheramy · 2024-11-14T21:29:53Z

While I greatly appreciate the effort that went into creating documentation for Oxidized (Thank you!), I must say I am quite overwhelmed by this PR.

My feeling is that you are doing too much at one time, resulting in a PR that is very difficult to handle:

rubocop does not pass
rake test does not pass
there are modifications (for example in spec/node_spec.rb) that do not seem to be related to documentation
I've found some code duplication somewhere - can't find it anymore - there are too much file changed [fixed by last commit]
The use of @!visibility private does not seem to be used in the intended way (hiding nicely written documentation)
when the points above will have been corrected, no one will be able to check what has been done

I suggest breaking the PR down into smaller steps (and PRs):

A first step would be to make a yard documentation and add yard to gemspec and Rakefile (rake doc). No code change, only documentation (even if yard complains and the result is ugly).
When this works, add a submodule for input
When this works, add a submodule for hooks
When this works, add a submodule for errors

Submodules for source and output have already been introduced by another PR - no work to do here.

I'm not sure I want submodules for models, as they are kind of a DSL and that makes writing models unnecessarily complicated. I'm not even sure I want a yard documentation for models, the yard output doesn't make a lot of sense (Instance Method Summary: #clean, #prompt).

The steps above could also be done as commits inside a branch, but I think it is easier to work with PR, as I can check and merge at each step, and changes done somewhere else in master will be included in the next step, reducing conflicts. The steps mentioned above are independent from each other an can be merged independently into master.

skiera6

Fixed more errors

skiera6 · 2024-11-14T23:16:47Z

My feeling is that you are doing too much at one time, resulting in a PR that is very difficult to handle:

I felt that might be an issue and you are right. I was wondering how i would go about it before starting the work i was thinking should i open an issue or an empty PR asking for the best way to go dealing with this

I've found some code duplication somewhere - can't find it anymore - there are too much file changed [fixed by last commit]

I think i found two more and fixed them

there are modifications (for example in spec/node_spec.rb) that do not seem to be related to documentation

Yeah i tried to make the tests pass after changing submodules and those were the leftovers of debugging now fixed

The use of @!visibility private does not seem to be used in the intended way (hiding nicely written documentation)

There's a lot of scattered comments that don't make sense and would make creating the yard docs harder instead of deleting them i opted to just make all of the comments private so they can be cleaned up later as the documentation gets better.

when the points above will have been corrected, no one will be able to check what has been done

Also one of the main issues i was thinking about you are right

When i'll find some time i will split this PR into separate PRs making it easier to deal with. Should i split the documentation into separate PRs/commits based on "submodules" to make it easier to deal with?

robertcheramy · 2024-11-15T19:37:09Z

Hello Eryk,

I think that if you go on with fixing the issues of this PR, it will just make more to split it afterwards in smaller PRs, which would be unnecessary work.
So I'm also fine with the approach you have taken so far - fix the issues of the PR, rebase or merge with main. When you are finished, mention me with @robertcheramy and I'll take a few cups of coffee and will review the Big PR.

skiera6 added 5 commits October 25, 2024 21:44

Added base for documentation.

95f5426

Changed namespaces in a way that will make for a better documentation. Separated errors into class. Made the rake test work.

Merge remote-tracking branch 'upstream/master'

1b94ffe

Wip merge to make the pull request easier

bc7a86d

Merged the current main branch and fixed tests

222cabc

Added documentation so yard shows 100% coverage (but it shows warning…

86cd2a9

…s about the dsl in models)

skiera6 changed the title ~~Yard docs~~ Base for Yard docs Oct 26, 2024

robertcheramy self-assigned this Nov 8, 2024

github-advanced-security bot found potential problems Nov 14, 2024

View reviewed changes

Fixed rubocop errors

2117fce

skiera6 commented Nov 14, 2024

View reviewed changes

Fixed more errors

c72935d

skiera6 and others added 2 commits November 15, 2024 00:37

Fixed minor issues preventing tests from passing

01476f0

Fixed rubocop errors

4520c91

skiera6 force-pushed the master branch from 032b52c to 4520c91 Compare November 16, 2024 03:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Base for Yard docs #3303

Base for Yard docs #3303

skiera6 commented Oct 26, 2024

skiera6 commented Nov 14, 2024

robertcheramy commented Nov 14, 2024

skiera6 left a comment

skiera6 commented Nov 14, 2024 •

edited

Loading

robertcheramy commented Nov 15, 2024 •

edited

Loading

Base for Yard docs #3303

Are you sure you want to change the base?

Base for Yard docs #3303

Conversation

skiera6 commented Oct 26, 2024

Pre-Request Checklist

Description

skiera6 commented Nov 14, 2024

robertcheramy commented Nov 14, 2024

skiera6 left a comment

Choose a reason for hiding this comment

skiera6 commented Nov 14, 2024 • edited Loading

robertcheramy commented Nov 15, 2024 • edited Loading

skiera6 commented Nov 14, 2024 •

edited

Loading

robertcheramy commented Nov 15, 2024 •

edited

Loading