Skip to content

Comments

lib: allow "show config running" command for non-transactional CLI#8073

Merged
Spantik merged 1 commit intoFRRouting:masterfrom
idryzhov:vtysh-show-config
Mar 1, 2021
Merged

lib: allow "show config running" command for non-transactional CLI#8073
Spantik merged 1 commit intoFRRouting:masterfrom
idryzhov:vtysh-show-config

Conversation

@idryzhov
Copy link
Contributor

This command doesn't rely on transactional CLI and works perfectly for
daemons converted to northbound configuration.

Signed-off-by: Igor Ryzhov iryzhov@nfware.com

This command doesn't rely on transactional CLI and works perfectly for
daemons converted to northbound configuration.

Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
Copy link

@polychaeta polychaeta left a 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 to FRR!

Click for style suggestions

To apply these suggestions:

curl -s https://gist.githubusercontent.com/polychaeta/18dc086258d630e7c4256e1a74df8bc5/raw/6fb75eed1003e07ea1ef3cacb552aedcc8f74098/cr_8073_1613143167.diff | git apply

diff --git a/vtysh/vtysh.c b/vtysh/vtysh.c
index 8ee8070d5..0fc5f1a91 100644
--- a/vtysh/vtysh.c
+++ b/vtysh/vtysh.c
@@ -2851,20 +2851,18 @@ DEFUN (vtysh_show_error_code,
 }
 
 /* Northbound. */
-DEFUN (show_config_running,
-       show_config_running_cmd,
-       "show configuration running\
+DEFUN(show_config_running, show_config_running_cmd,
+      "show configuration running\
           [<json|xml> [translate WORD]]\
           [with-defaults]" DAEMONS_LIST,
-       SHOW_STR
-       "Configuration information\n"
-       "Running configuration\n"
-       "Change output format to JSON\n"
-       "Change output format to XML\n"
-       "Translate output\n"
-       "YANG module translator\n"
-       "Show default values\n"
-       DAEMONS_STR)
+      SHOW_STR
+      "Configuration information\n"
+      "Running configuration\n"
+      "Change output format to JSON\n"
+      "Change output format to XML\n"
+      "Translate output\n"
+      "YANG module translator\n"
+      "Show default values\n" DAEMONS_STR)
 {
 	return show_one_daemon(vty, argv, argc - 1, argv[argc - 1]->text);
 }

If you are a new contributor to FRR, please see our contributing guidelines.

After making changes, you do not need to create a new PR. You should perform an amend or interactive rebase followed by a force push.

@LabN-CI
Copy link
Collaborator

LabN-CI commented Feb 12, 2021

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

Results table
_ _
Result SUCCESS git merge/8073 bcbe60d
Date 02/12/2021
Start 11:20:20
Finish 11:59:50
Run-Time 39:30
Total 1815
Pass 1815
Fail 0
Valgrind-Errors 0
Valgrind-Loss 0
Details vncregress-2021-02-12-11:20:20.txt
Log autoscript-2021-02-12-11:21:23.log.bz2
Memory 504 486 428

For details, please contact louberger

@NetDEF-CI
Copy link
Collaborator

Continuous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17107/

This is a comment from an automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

Warnings Generated during build:

Debian 10 amd64 build: Successful with additional warnings

Debian Package lintian failed for Debian 10 amd64 build:
(see full package build log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17107/artifact/DEB10BUILD/ErrorLog/log_lintian.txt)

W: frr source: pkg-js-tools-test-is-missing
W: frr source: newer-standards-version 4.5.0.3 (current is 4.3.0)
W: frr source: pkg-js-tools-test-is-missing
W: frr source: newer-standards-version 4.5.0.3 (current is 4.3.0)
W: frr-rpki-rtrlib: changelog-file-missing-explicit-entry 7.5-0 -> 7.7-dev-20210212-00-gbcbe60d45-0 (missing) -> 7.7-dev-20210212-00-gbcbe60d45-0~deb10u1
W: frr: changelog-file-missing-explicit-entry 7.5-0 -> 7.7-dev-20210212-00-gbcbe60d45-0 (missing) -> 7.7-dev-20210212-00-gbcbe60d45-0~deb10u1
W: frr-snmp: changelog-file-missing-explicit-entry 7.5-0 -> 7.7-dev-20210212-00-gbcbe60d45-0 (missing) -> 7.7-dev-20210212-00-gbcbe60d45-0~deb10u1
W: frr-pythontools: changelog-file-missing-explicit-entry 7.5-0 -> 7.7-dev-20210212-00-gbcbe60d45-0 (missing) -> 7.7-dev-20210212-00-gbcbe60d45-0~deb10u1
W: frr-doc: changelog-file-missing-explicit-entry 7.5-0 -> 7.7-dev-20210212-00-gbcbe60d45-0 (missing) -> 7.7-dev-20210212-00-gbcbe60d45-0~deb10u1

@pushpasis
Copy link
Contributor

The changes look pretty straightforward and okay to me. But I am now wondering why did we introduce the command 'show config running' command when there was already 'show running-config'. Offcourse that has nothing to do with the changes n this PR.

Copy link
Contributor

@pushpasis pushpasis left a comment

Choose a reason for hiding this comment

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

Changes look straightforward and okay to me.

Copy link
Member

@Spantik Spantik left a comment

Choose a reason for hiding this comment

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

LGTM. Do we have document updated for this comamnd?

@idryzhov
Copy link
Contributor Author

@Spantik there are no docs for northbound CLI commands at all

@Spantik
Copy link
Member

Spantik commented Mar 1, 2021

Ok we can get this in for now and add documention for all new commands introduced as part of NB.

@Spantik Spantik merged commit 0a1e7b6 into FRRouting:master Mar 1, 2021
@rwestphal
Copy link
Member

The changes look pretty straightforward and okay to me. But I am now wondering why did we introduce the command 'show config running' command when there was already 'show running-config'. Offcourse that has nothing to do with the changes n this PR.

show config running was added for a few reasons:

  • It shows only the YANG-modeled configuration. This is because the configuration is displayed based solely on the NB cli_show() callbacks (more details here);
  • It has parameters that wouldn't make sense for show running-config, like "format", "translate" and others;
  • It's the counterpart of show config candidate.

show config running was intentionally not made available in vtysh because it's only useful for developers as of now (not all config commands were YANGified yet).

Users will probably get confused when they see that there are two different commands to display the running configuration, and one of them works only partially. Now that this PR was merged, I think it'd be a good idea to either revert it or mark the command as hidden.

@idryzhov
Copy link
Contributor Author

@rwestphal I'll make it hidden. No need to revert, it is very useful for debugging even with non-transactional CLI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants