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
isisd: fix show isis database [detail] json #11906
base: master
Are you sure you want to change the base?
Conversation
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: FailedFedora 29 amd64 build: Failed (click for details)Make failed for Fedora 29 amd64 build:
Fedora 29 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-7234/artifact/F29BUILD/config.status/config.status Ubuntu 20.04 amd64 build: Failed (click for details)Ubuntu 20.04 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-7234/artifact/U2004AMD64BUILD/config.status/config.statusMake failed for Ubuntu 20.04 amd64 build:
Ubuntu 20.04 amd64 build: Unknown Log <config.log.gz> OpenBSD 7 amd64 build: Failed (click for details)Make failed for OpenBSD 7 amd64 build:
OpenBSD 7 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-7234/artifact/CI011BUILD/config.status/config.status Ubuntu 18.04 arm8 build: Failed (click for details)Ubuntu 18.04 arm8 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-7234/artifact/U18ARM8BUILD/config.status/config.status Ubuntu 18.04 arm8 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-7234/artifact/U18ARM8BUILD/config.log/config.log.gzMake failed for Ubuntu 18.04 arm8 build:
Debian 9 amd64 build: Failed (click for details)Make failed for Debian 9 amd64 build:
Debian 9 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-7234/artifact/CI021BUILD/config.status/config.status Redhat 8 amd64 build: Failed (click for details)Redhat 8 amd64 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-7234/artifact/REDHAT8/config.log/config.log.gzMake failed for Redhat 8 amd64 build:
Redhat 8 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-7234/artifact/REDHAT8/config.status/config.status Redhat 9 amd64 build: Failed (click for details)Redhat 9 amd64 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-7234/artifact/RH9BUILD/config.log/config.log.gzMake failed for Redhat 9 amd64 build:
Redhat 9 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-7234/artifact/RH9BUILD/config.status/config.status Debian 10 amd64 build: Failed (click for details)Debian 10 amd64 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-7234/artifact/DEB10BUILD/config.log/config.log.gzMake failed for Debian 10 amd64 build:
Debian 10 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-7234/artifact/DEB10BUILD/config.status/config.status Ubuntu 18.04 ppc64le build: Failed (click for details)Make failed for Ubuntu 18.04 ppc64le build:
Ubuntu 18.04 ppc64le build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-7234/artifact/U1804PPC64LEBUILD/config.status/config.status Ubuntu 16.04 arm7 build: Failed (click for details)Ubuntu 16.04 arm7 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-7234/artifact/CI101BUILD/config.status/config.status Ubuntu 16.04 arm7 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-7234/artifact/CI101BUILD/config.log/config.log.gzMake failed for Ubuntu 16.04 arm7 build:
FreeBSD 11 amd64 build: Failed (click for details)Make failed for FreeBSD 11 amd64 build:
FreeBSD 11 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-7234/artifact/CI009BUILD/config.status/config.status Ubuntu 22.04 amd64 build: Failed (click for details)Ubuntu 22.04 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-7234/artifact/U22AMD64BUILD/config.status/config.statusMake failed for Ubuntu 22.04 amd64 build:
Ubuntu 22.04 amd64 build: Unknown Log <config.log.gz> Ubuntu 16.04 arm8 build: Failed (click for details)Make failed for Ubuntu 16.04 arm8 build:
Ubuntu 16.04 arm8 build: Unknown Log <config.log.gz> Ubuntu 18.04 amd64 build: Failed (click for details)Make failed for Ubuntu 18.04 amd64 build:
Ubuntu 18.04 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-7234/artifact/U1804AMD64/config.status/config.status Debian 11 amd64 build: Failed (click for details)Debian 11 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-7234/artifact/DEB11AMD64/config.status/config.statusMake failed for Debian 11 amd64 build:
Debian 11 amd64 build: Unknown Log <config.log.gz> Ubuntu 18.04 i386 build: Failed (click for details)Ubuntu 18.04 i386 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-7234/artifact/U18I386BUILD/config.log/config.log.gz Ubuntu 18.04 i386 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-7234/artifact/U18I386BUILD/config.status/config.statusMake failed for Ubuntu 18.04 i386 build:
NetBSD 9 amd64 build: Failed (click for details)NetBSD 9 amd64 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-7234/artifact/CI012BUILD/config.log/config.log.gzMake failed for NetBSD 9 amd64 build:
NetBSD 9 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-7234/artifact/CI012BUILD/config.status/config.status Ubuntu 18.04 arm7 build: Failed (click for details)Make failed for Ubuntu 18.04 arm7 build:
Ubuntu 18.04 arm7 build: Unknown Log <config.log.gz> Ubuntu 16.04 amd64 build: Failed (click for details)Make failed for Ubuntu 16.04 amd64 build:
Ubuntu 16.04 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-7234/artifact/CI014BUILD/config.status/config.status Ubuntu 16.04 i386 build: Failed (click for details)Make failed for Ubuntu 16.04 i386 build:
Ubuntu 16.04 i386 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-7234/artifact/U1604I386/config.status/config.status FreeBSD 12 amd64 build: Failed (click for details)Make failed for FreeBSD 12 amd64 build:
FreeBSD 12 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-7234/artifact/FBSD12AMD64/config.status/config.status Successful on other platforms/tests
Warnings Generated during build:Checkout code: Successful with additional warningsFedora 29 amd64 build: Failed (click for details)Make failed for Fedora 29 amd64 build:
Fedora 29 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-7234/artifact/F29BUILD/config.status/config.status Ubuntu 20.04 amd64 build: Failed (click for details)Ubuntu 20.04 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-7234/artifact/U2004AMD64BUILD/config.status/config.statusMake failed for Ubuntu 20.04 amd64 build:
Ubuntu 20.04 amd64 build: Unknown Log <config.log.gz> OpenBSD 7 amd64 build: Failed (click for details)Make failed for OpenBSD 7 amd64 build:
OpenBSD 7 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-7234/artifact/CI011BUILD/config.status/config.status Ubuntu 18.04 arm8 build: Failed (click for details)Ubuntu 18.04 arm8 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-7234/artifact/U18ARM8BUILD/config.status/config.status Ubuntu 18.04 arm8 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-7234/artifact/U18ARM8BUILD/config.log/config.log.gzMake failed for Ubuntu 18.04 arm8 build:
Debian 9 amd64 build: Failed (click for details)Make failed for Debian 9 amd64 build:
Debian 9 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-7234/artifact/CI021BUILD/config.status/config.status Redhat 8 amd64 build: Failed (click for details)Redhat 8 amd64 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-7234/artifact/REDHAT8/config.log/config.log.gzMake failed for Redhat 8 amd64 build:
Redhat 8 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-7234/artifact/REDHAT8/config.status/config.status Redhat 9 amd64 build: Failed (click for details)Redhat 9 amd64 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-7234/artifact/RH9BUILD/config.log/config.log.gzMake failed for Redhat 9 amd64 build:
Redhat 9 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-7234/artifact/RH9BUILD/config.status/config.status Debian 10 amd64 build: Failed (click for details)Debian 10 amd64 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-7234/artifact/DEB10BUILD/config.log/config.log.gzMake failed for Debian 10 amd64 build:
Debian 10 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-7234/artifact/DEB10BUILD/config.status/config.status Ubuntu 18.04 ppc64le build: Failed (click for details)Make failed for Ubuntu 18.04 ppc64le build:
Ubuntu 18.04 ppc64le build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-7234/artifact/U1804PPC64LEBUILD/config.status/config.status Ubuntu 16.04 arm7 build: Failed (click for details)Ubuntu 16.04 arm7 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-7234/artifact/CI101BUILD/config.status/config.status Ubuntu 16.04 arm7 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-7234/artifact/CI101BUILD/config.log/config.log.gzMake failed for Ubuntu 16.04 arm7 build:
FreeBSD 11 amd64 build: Failed (click for details)Make failed for FreeBSD 11 amd64 build:
FreeBSD 11 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-7234/artifact/CI009BUILD/config.status/config.status Ubuntu 22.04 amd64 build: Failed (click for details)Ubuntu 22.04 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-7234/artifact/U22AMD64BUILD/config.status/config.statusMake failed for Ubuntu 22.04 amd64 build:
Ubuntu 22.04 amd64 build: Unknown Log <config.log.gz> Ubuntu 16.04 arm8 build: Failed (click for details)Make failed for Ubuntu 16.04 arm8 build:
Ubuntu 16.04 arm8 build: Unknown Log <config.log.gz> Ubuntu 18.04 amd64 build: Failed (click for details)Make failed for Ubuntu 18.04 amd64 build:
Ubuntu 18.04 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-7234/artifact/U1804AMD64/config.status/config.status Debian 11 amd64 build: Failed (click for details)Debian 11 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-7234/artifact/DEB11AMD64/config.status/config.statusMake failed for Debian 11 amd64 build:
Debian 11 amd64 build: Unknown Log <config.log.gz> Ubuntu 18.04 i386 build: Failed (click for details)Ubuntu 18.04 i386 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-7234/artifact/U18I386BUILD/config.log/config.log.gz Ubuntu 18.04 i386 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-7234/artifact/U18I386BUILD/config.status/config.statusMake failed for Ubuntu 18.04 i386 build:
NetBSD 9 amd64 build: Failed (click for details)NetBSD 9 amd64 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-7234/artifact/CI012BUILD/config.log/config.log.gzMake failed for NetBSD 9 amd64 build:
NetBSD 9 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-7234/artifact/CI012BUILD/config.status/config.status Ubuntu 18.04 arm7 build: Failed (click for details)Make failed for Ubuntu 18.04 arm7 build:
Ubuntu 18.04 arm7 build: Unknown Log <config.log.gz> Ubuntu 16.04 amd64 build: Failed (click for details)Make failed for Ubuntu 16.04 amd64 build:
Ubuntu 16.04 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-7234/artifact/CI014BUILD/config.status/config.status Ubuntu 16.04 i386 build: Failed (click for details)Make failed for Ubuntu 16.04 i386 build:
Ubuntu 16.04 i386 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-7234/artifact/U1604I386/config.status/config.status FreeBSD 12 amd64 build: Failed (click for details)Make failed for FreeBSD 12 amd64 build:
FreeBSD 12 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-7234/artifact/FBSD12AMD64/config.status/config.status
|
183cbc6
to
795be31
Compare
Continuous Integration Result: SUCCESSFULContinuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-7239/ This is a comment from an automated CI system. |
Continuous Integration Result: SUCCESSFULContinuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-7238/ This is a comment from an automated CI system. Warnings Generated during build:Checkout code: Successful with additional warnings
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some comments about backward compatibility and JSON key naming.
@@ -494,36 +494,36 @@ static void format_item_ext_subtlvs(struct isis_ext_subtlvs *exts, | |||
adj->sid); | |||
json_object_int_add(flags_json, "weight", | |||
adj->weight); | |||
json_object_string_add( | |||
json_object_boolean_add( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically this is correct, but should we keep both formats (1/0, true/false) for a deprecation cycle?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, the JSON output of this command was not usable because the information was wrong.
For example, "show isis database json" was mixing info of all routers in a single object {}.
Even with a single router "show isis database RTR json", information was not in the right place. If multiple TLVs
were present, only the last one was displayed but sometimes with data from the previous ones.
I think it is not useful to have a deprecation cycle.
isisd/isis_tlvs.c
Outdated
} | ||
json_object_array_add(array_json, old_json); | ||
json_object_object_add(json, "is-reach", old_json); | ||
json_object_string_add(old_json, "is-neighbor", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the specific TLV name or can be anything? If anything, then let's do not follow old naming and use as it should be correctly (e.g.:: isNeighbor).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
old_json = json_object_new_object(); | ||
json_object_object_add(json, "old-ip-reach-style", old_json); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we breaking backward compatibility here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The command was not working before
isisd/isis_tlvs.c
Outdated
json_object_int_add(mt_json, "mtid", info->mtid); | ||
json_object_string_add(mt_json, "mt-description", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto, regarding JSON key naming.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the places where @ton31337 has pointed out backward compatibility problems need to be addressed ... otherwise the code looks fine
795be31
to
c5ccb09
Compare
The command was not working so we can consider there is no backward compatibility problems |
c5ccb09
to
313be35
Compare
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: FailedUbuntu 16.04 arm8 build: Failed (click for details)Ubuntu 16.04 arm8 build: No useful log foundSuccessful on other platforms/tests
|
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopotests Ubuntu 18.04 i386 part 7: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO7U18I386-8910/test Topology Tests failed for Topotests Ubuntu 18.04 i386 part 7 Topotests Ubuntu 18.04 amd64 part 7: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO7U18AMD64-8910/test Topology Tests failed for Topotests Ubuntu 18.04 amd64 part 7 Topotests Ubuntu 18.04 arm8 part 7: Failed (click for details)Topotests Ubuntu 18.04 arm8 part 7: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-8910/artifact/TOPO7U18ARM8/TopotestDetails/ Topotests Ubuntu 18.04 arm8 part 7: No useful log foundTopotests debian 10 amd64 part 7: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO7DEB10AMD64-8910/test Topology Tests failed for Topotests debian 10 amd64 part 7 Successful on other platforms/tests
|
Information about the routers in lsdb are mixed in the same json as if it was about the same router. Display all values in an array. Fixes: a2cac12 ("isisd: Add json to show isis database command.") Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
Display information about a particular router in show isis database in an single value array for compatibility with the display of all routers. Fixes: a2cac12 ("isisd: Add json to show isis database command.") Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
Multiple occurrences of the same (sub-)TLVs are mixed into a single JSON object. Last values replaces the previous. Display all the occurrences in an array for the following (sub-)TLVs: - is-reach - ip-reach - ext-reach - ext-ip-reach - ipv6-reach - MT - prefix-SID Fixes: a2cac12 ("isisd: Add json to show isis database command.") Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
Some keys are wrongly displayed at the JSON parent level. Add the key at the current level. Fixes: a2cac12 ("isisd: Add json to show isis database command.") Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
Only the MT ID is displayed. Display the MT description as well. Fixes: a2cac12 ("isisd: Add json to show isis database command.") Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
When a level is not present in show isis database detail json, {} is displayed. Display nothing for non present level. Fixes: a2cac12 ("isisd: Add json to show isis database command.") Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
Display booleans values with string such as "", "*", "yes" and "no" are not correct on JSON. Display boolean values as true or false. Fixes: a2cac12 ("isisd: Add json to show isis database command.") Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
JSON keys should not contain '-'. For example, "flag-v" is incorrect and should be "flagV". Rename JSON keys in "show isis database detail json" that contains '-' or '_'. Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
{"mtRouterInfo": "none"} JSON is incorrect. Change to {"mtRouterInfo": null} Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
313be35
to
b2fcdaa
Compare
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-8990/ This is a comment from an automated CI system. CLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base2 Static Analyzer issues remaining.See details at |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
This PR is stale because it has been open 180 days with no activity. Comment or remove the |
Fixes for show isis database [detail] json