-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
lib: yang defination for basic routing frr-routing #5530
Conversation
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.
Thanks for your contribution to FRR!
- One of your commits has a missing or badly formatted
Signed-off-by
line; we can't accept your contribution until all of your commits have one
If you are a new contributor to FRR, please see our contributing guidelines.
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-FRRPULLREQ-10085/ This is a comment from an automated CI system. Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build:
CLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base1 Static Analyzer issues remaining.See details at |
This is going nowhere until the signoff line is fixed. |
eec5d7b
to
6b3bd6f
Compare
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.
Thanks for your contribution to FRR!
- One of your commits has a missing or badly formatted
Signed-off-by
line; we can't accept your contribution until all of your commits have one
If you are a new contributor to FRR, please see our contributing guidelines.
6b3bd6f
to
3ccf8c1
Compare
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.
Thanks for your contribution to FRR!
- One of your commits has a missing or badly formatted
Signed-off-by
line; we can't accept your contribution until all of your commits have one
If you are a new contributor to FRR, please see our contributing guidelines.
3ccf8c1
to
2518f56
Compare
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.
Thanks for your contribution to FRR!
- One of your commits has a missing or badly formatted
Signed-off-by
line; we can't accept your contribution until all of your commits have one
If you are a new contributor to FRR, please see our contributing guidelines.
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: FailedFreeBSD 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-FRRPULLREQ-10173/artifact/CI009BUILD/config.status/config.status Successful on other platforms
|
2518f56
to
942443e
Compare
blocking comments addressed
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: FailedFreeBSD 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-FRRPULLREQ-10178/artifact/CI009BUILD/config.status/config.status Successful on other platforms
Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build:
|
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-FRRPULLREQ-10172/ This is a comment from an automated CI system. CLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base1 Static Analyzer issues remaining.See details at |
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-FRRPULLREQ-10177/ This is a comment from an automated CI system. CLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base1 Static Analyzer issues remaining.See details at |
ci:rerun |
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: FailedFreeBSD 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-FRRPULLREQ-10532/artifact/CI009BUILD/config.status/config.status Successful on other platforms
Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build:
|
942443e
to
a32359a
Compare
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: FailedFreeBSD 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-FRRPULLREQ-10534/artifact/CI009BUILD/config.status/config.status Successful on other platforms
|
I am getting below error after running below commands: git clone https://github.com/vishaldhingra/frr.git I feel it's a rights issue. Please find the logs Logs :
|
a32359a
to
dbb8900
Compare
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: FailedFreeBSD 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-FRRPULLREQ-10543/artifact/CI009BUILD/config.status/config.status Successful on other platforms
Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build:
|
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-FRRPULLREQ-10543/ This is a comment from an automated CI system. Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build:
CLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base1 Static Analyzer issues remaining.See details at |
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 a few comments. IMO our YANG description
s should describe the objects being modeled, not the model itself.
yang/frr-routing.yang
Outdated
} | ||
description | ||
"Type of the control-plane protocol -- an identity | ||
derived from the 'control-plane-protocol' |
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.
Let's not be too detailed in the descriptions; they are used to add information that isn't immediately obvious from the model itself. Plus if the name of the base ever changes the description will be out of date and the model will still validate.
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.
done
yang/frr-routing.yang
Outdated
leaf vrf { | ||
type frr-vrf:vrf-ref; | ||
description | ||
"routing instance"; |
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.
VRF of routing instance
?
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.
done
yang/frr-routing.yang
Outdated
} | ||
|
||
identity static { | ||
base routing-protocol; |
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 believe this identity could be defined in the staticd module instead (which should augment this one).
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.
done, It's part of static yang now.
yang/frr-routing.yang
Outdated
} | ||
|
||
identity direct { | ||
base routing-protocol; |
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.
We should also have identities to the other base route types: kernel and system (?). And maybe we could name this connected
instead since this is the term most people are used to.
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 have removed this identity, as it's a route type. For route type frr-route-types.yang is already there.
yang/frr-routing.yang
Outdated
/* Type Definitions */ | ||
|
||
typedef route-preference { | ||
type uint32; |
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.
Rename to administrative-distance
? Or at least make it clear what this means in the description (I wouldn't be shy to reuse the original description from the IETF module).
Also, this needs to be uint8
for FRR.
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.
done
e7860ed
to
c16f9f5
Compare
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: FailedFreeBSD 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-FRRPULLREQ-10659/artifact/CI009BUILD/config.status/config.status Successful on other platforms
Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build:
|
@rwestphal and @qlyoung thanks for your review. |
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-FRRPULLREQ-10660/ This is a comment from an automated CI system. Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build:
|
lib/subdir.am
Outdated
@@ -106,6 +106,7 @@ nodist_lib_libfrr_la_SOURCES = \ | |||
yang/frr-interface.yang.c \ | |||
yang/frr-route-types.yang.c \ | |||
yang/frr-vrf.yang.c \ | |||
yang/frr-routing.yang.c \ |
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.
Please fix the formatting here; use tabs
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.
done
Yang files for basic frr-routing used by other daemons like staticd and pim Co-authored-by: Santosh P K <sapk@vmware.com> Co-authored-by: vishaldhingra <vdhingra@vmware.com> Signed-off-by: vishaldhingra <vdhingra@vmware.com>
c16f9f5
to
f5319ff
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-FRRPULLREQ-10671/ This is a comment from an automated CI system. |
Yang files for basic frr-routing used by other
daemons like staticd and pim
Co-authored-by: Santosh P K sapk@vmware.com
Co-authored-by: vishaldhingra vdhingra@vmware.com
Signed-off-by: vishaldhingra vdhingra@vmware.com