-
Notifications
You must be signed in to change notification settings - Fork 326
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
T689: updated show host xml and fixed op_mode script names from last PR. #17
Conversation
Can one of the admins verify this patch? |
Microsoft Github 98 won't let me leave review comments in this pull request due to a glitch, so I have to write them here. "show host date" — why duplicate "show date" from show-date.xml? "show host lookup" — oh, now this is interesting. Established convention is that show commands only display already available information, from the UI consistency perspective lookup doesn't belong in there. |
Hi! Completely inline with you on that! "show host date" <-- no reason to duplicate this, also "show date" is easier.. i'll remove it from the code. "show host name" and "show host domain" i cant find any other place to put this. "show host lookup" <-- agreed, its not a show command.. and should not be.. but i don't know where it should be located. and as you noted... will it be used? what it actually does is testing that DNS is functioning.. so, maybe a "test" top level "test dns-lookup ##" ? do we have anything else that could be moved into a new "test" top level command? (test ntp peer ##, test radius.. etc.) |
My point is that "show host os" is very unlikely to be useful: it's faster and more familiar to just use uname with desired options than to remember what that "native" VyOS command was. I also doubt that kernel version is very useful, since "add system image" is the only supported update method and kernel versions of official images are known (and if someone updates the kernel by hand, or makes a custom image, they definitely know enough to verify it by hand — or they shouldn't be doing it). As of the top level word for DNS lookup, I think it warrants a broader discussion. Still, I'm sceptical of its usefulness. To verify that VyOS can look up names, one can simply do "ping vyos.net". |
se your point on "Show host os", i'll remove it. when were moving towards vyos 2.x and are using the vyconf interpreter instead of vbash, will native linux commands still be available? or do wee need to switch shell to "sh" to use native linux commands? if that is the case i would like to opt them in, because entering another shell to use simple things like ping etc. is not very user friendly. for a DNS lookup test, yea wee need to have a option to enter DNS server address. the same with all other protocol tests. |
…ink they know about 'uname -a' and because upgrading kernel in vyos is not allowed outsude the release schedule kernel information shoild be maintained inside the vyos install package.
Sorry for late reply! Ok, I think let's keep the lookup. Since it's the kind op mode commands that rarely if ever will be used from scripts, I think we can always change it later. |
Fixing IPv6 next-hop in route-map
as noted in the phabricator case after my last PR filenames on op_mode scripts was with dash'es insted of underscore. They are now fixed and i also added a rewritten "show host *" xml.