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

Base for Yard docs #3303

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Base for Yard docs #3303

wants to merge 9 commits into from

Conversation

skiera6
Copy link

@skiera6 skiera6 commented Oct 26, 2024

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.

@skiera6 skiera6 changed the title Yard docs Base for Yard docs Oct 26, 2024
@robertcheramy robertcheramy self-assigned this Nov 8, 2024
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

Check failure

Code scanning / CodeQL

Polynomial regular expression used on uncontrolled data High

This
regular expression
that depends on a
library input
may run slow on strings with many repetitions of '0'.
cmd 'show system information' do |cfg|
cfg.sub! /^Device Up Time.*\n/, ''
cfg.delete! "\r"
comment(cfg).gsub(/ +$/, '')

Check failure

Code scanning / CodeQL

Polynomial regular expression used on uncontrolled data High

This
regular expression
that depends on a
library input
may run slow on strings with many repetitions of ' '.
Comment on lines +57 to +68
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(/ +$/, '')

Check failure

Code scanning / CodeQL

Polynomial regular expression used on uncontrolled data High

This
regular expression
that depends on a
library input
may run slow on strings with many repetitions of ' '.
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>'

Check failure

Code scanning / CodeQL

Inefficient regular expression High

This part of the regular expression may cause exponential backtracking on strings starting with 'aaa-server tacacs (!) host\n' and containing many repetitions of '\t\t\n'.
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>'

Check failure

Code scanning / CodeQL

Inefficient regular expression High

This part of the regular expression may cause exponential backtracking on strings starting with 'aaa-server ! (!) host\n' and containing many repetitions of '\t\t\n'.
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>'

Check failure

Code scanning / CodeQL

Inefficient regular expression High

This part of the regular expression may cause exponential backtracking on strings containing many repetitions of '\t\t\n'.
# @!method prompt(regex)
# Sets the prompt for the device.
# @param regex [Regexp] The regular expression that matches the prompt.
prompt /^\((\w|\S)+\) (>|#)$/

Check failure

Code scanning / CodeQL

Inefficient regular expression High

This part of the regular expression may cause exponential backtracking on strings starting with '(' and containing many repetitions of '0'.
cmd 'show run' do |cfg|
cfg.each_line.select do |line|
(not line.match /^!.*$/) &&
(not line.match /^\((\w|\S)+\) (>|#)$/) &&

Check failure

Code scanning / CodeQL

Inefficient regular expression High

This part of the regular expression may cause exponential backtracking on strings starting with '(' and containing many repetitions of '0'.
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)?)*)"/

Check failure

Code scanning / CodeQL

Polynomial regular expression used on uncontrolled data High

This
regular expression
that depends on a
library input
may run slow on strings starting with 'system-uptime "' and with many repetitions of '00'.
This
regular expression
that depends on a
library input
may run slow on strings starting with 'system-uptime "0\t' and with many repetitions of '00\t00'.
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)?)*)"/

Check failure

Code scanning / CodeQL

Inefficient regular expression High

This part of the regular expression may cause exponential backtracking on strings starting with 'system-uptime "0\t' and containing many repetitions of '000\t'.
@skiera6
Copy link
Author

skiera6 commented Nov 14, 2024

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
Copy link
Collaborator

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.

Copy link
Author

@skiera6 skiera6 left a comment

Choose a reason for hiding this comment

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

Fixed more errors

@skiera6
Copy link
Author

skiera6 commented Nov 14, 2024

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?

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