feat: Add Cisco IOS-XR Provider and implement interface stubs #85
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sven-rosenzweig
force-pushed
the
feat/iosxrv2
branch
2 times, most recently
from
120c4a7 to
ff39cee
Compare
Contributor
Author
|
Waiting for #30 to be merged. |
sven-rosenzweig
force-pushed
the
feat/iosxrv2
branch
from
ff39cee to
b1a8ff0
Compare
hardikdr
added
the
area/metal-automation
sven-rosenzweig
force-pushed
the
feat/iosxrv2
branch
2 times, most recently
from
32662d3 to
35d7a78
Compare
Some network devices do not have TLS configured on their side, requiring the use of insecure credentials. To support these cases, this change disables transport security enforcement when insecure credentials are used. Note: This option is intended for development and testing purposes only.
Refactor the Provider to depend on a Client interface instead of a concrete implementation. This simplifies testing and mocking by allowing the Client itself to be mocked, rather than individual gRPC calls.
sven-rosenzweig
force-pushed
the
feat/iosxrv2
branch
2 times, most recently
from
f3ee185 to
0895369
Compare
sven-rosenzweig
force-pushed
the
feat/iosxrv2
branch
from
0895369 to
9c61755
Compare
Merging this branch changes the coverage (1 decrease, 1 increase)
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
| } | ||
|
|
||
| func (a *auth) RequireTransportSecurity() bool { return true } | ||
| func (a *auth) RequireTransportSecurity() bool { |
Contributor
There was a problem hiding this comment.
nit: we could just always use false here as this is only called with the insecure transport credentials.
Comment on lines
66
to
+68
| // Client is a gNMI client offering convenience methods for device configuration | ||
| // using gNMI. | ||
| type Client struct { | ||
| type ClientObj struct { |
Contributor
There was a problem hiding this comment.
Suggested change
| // Client is a gNMI client offering convenience methods for device configuration | |
| // using gNMI. | |
| type Client struct { | |
| type ClientObj struct { | |
| type client struct { |
Would move the comment to the Client interface definition and make the client implementation lowercase (unexported).
Contributor
There was a problem hiding this comment.
nit: seems like this file is inconsistency indented. Maybe re-indent.
| ) | ||
|
|
||
| // YANG has an empty type that is represented as a JSON array with a single null value. Whenever the key is present in YANG, it should evaluate to true. | ||
| type Empty bool |
Contributor
There was a problem hiding this comment.
This type is already contained in the gnmiext package and can be used from there.
| return nil, nil | ||
| } | ||
|
|
||
| type PhisIf struct { |
Contributor
There was a problem hiding this comment.
Suggested change
| type PhisIf struct { | |
| type PhysIf struct { |
| // (fixme): for the moment it is enought to keep this static | ||
| // option1: extend existing interface spec | ||
| // option2: create a custom iosxr config | ||
| physif.Shutdown = Empty(false) |
Contributor
There was a problem hiding this comment.
Suggested change
| physif.Shutdown = Empty(false) | |
| physif.Shutdown = Empty(false) | |
| if req.Interface.Spec.AdminState == v1alpha1.InterfaceAdminStateDown { | |
| physif.Shutdown = Empty(true) | |
| } |
| return p.conn.Close() | ||
| } | ||
|
|
||
| func (p *Provider) EnsureInterface(ctx context.Context, req *provider.InterfaceRequest) error { |
Contributor
There was a problem hiding this comment.
Currently only Physical Interfaces are supported. Maybe we should return an error of the Type doesn't match.
| if err != nil { | ||
| return fmt.Errorf("failed to create interface %s: %w", req.Interface.Spec.Name, err) | ||
| } | ||
| fmt.Printf("Interface %s created successfully\n", req.Interface.Spec.Name) |
Contributor
There was a problem hiding this comment.
Suggested change
| fmt.Printf("Interface %s created successfully\n", req.Interface.Spec.Name) |
| return nil | ||
| } | ||
|
|
||
| err = p.client.Patch(ctx, physif) |
Contributor
There was a problem hiding this comment.
Suggested change
| err = p.client.Patch(ctx, physif) | |
| return p.client.Update(ctx, physif) |
I think I would just always use a Update, then you also don't need the check above.
Comment on lines
+147
to
+155
| providerStatus := provider.InterfaceStatus{ | ||
| OperStatus: true, | ||
| } | ||
|
|
||
| if stateMapping[state.State] != StateUp { | ||
| providerStatus.OperStatus = false | ||
| } | ||
|
|
||
| return providerStatus, nil |
Contributor
There was a problem hiding this comment.
Suggested change
| providerStatus := provider.InterfaceStatus{ | |
| OperStatus: true, | |
| } | |
| if stateMapping[state.State] != StateUp { | |
| providerStatus.OperStatus = false | |
| } | |
| return providerStatus, nil | |
| return provider.InterfaceStatus{ | |
| OperStatus: state.State == StateUp, | |
| }, nil |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.