[WORK IN PROGRESS] Add support to manage multiple switches#50
Open
davidgengenbach wants to merge 1 commit intop4lang:mainfrom
Open
[WORK IN PROGRESS] Add support to manage multiple switches#50davidgengenbach wants to merge 1 commit intop4lang:mainfrom
davidgengenbach wants to merge 1 commit intop4lang:mainfrom
Conversation
Contributor
Author
|
By the way, Travis does not build the PR because
https://travis-ci.com/github/p4lang/p4runtime-shell/requests The tests passed locally. |
davidgengenbach
force-pushed
the
master
branch
4 times, most recently
from
ced0253 to
1e3aada
Compare
davidgengenbach
force-pushed
the
master
branch
from
1e3aada to
1487294
Compare
antoninbas
reviewed
Dec 2, 2020
Member
antoninbas
left a comment
antoninbas
left a comment
There was a problem hiding this comment.
sorry for the delay
just one comment, otherwise LGTM.
not sure how you want to proceed for the unit tests: either 1) the current tests can be converted to stop relying on the global client (and we can add one simple test to make sure the global client still works), or 2) we can keep the current tests as they are and add a simple test for the "scripting" use case
| logging.basicConfig(level=logging.DEBUG) | ||
|
|
||
| setup(args.device_id, args.grpc_addr, args.election_id, args.config) | ||
| setup(args.device_id, args.grpc_addr, args.election_id, args.config, set_global_client=True) |
Member
There was a problem hiding this comment.
I feel like set_global_client is actually not needed in setup:
- when using p4runtime_sh for scripts, there is no need for global client support IMO
- when using p4runtime_sh as a CLI, setting the global client can be done here, in the
mainfunction
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
@antoninbas I started de-coupling the
client/contextfields. Here is my current approach which should not break the current prompt.However, there are breaking changes for people who use scripting:
p4runtime_sh.shell.clientwas renamed top4runtime_sh.shell.global_clientp4_runtime_sh.shell.contextwas moved into thecontextfield onP4RuntimeClientSo, if library users use
p4_runtime_sh.shell.clientandp4_runtime_sh.shell.contextdirectly (which is not documented in the README), they will get undefined-errors.While this is a breaking change, I am convinced that moving the
Contextinto theP4RuntimeClientis the right way since the context is closely coupled to the client. For example, when setting the forwarding config via theP4RuntimeClient, theContextshould be adapted as well.To make clear that we introduce "breaking" changes, we can introduce versioning, e.g., by tagging the repo in the current state as
v1and directly moving tov2with my changes.🚧 Important: these changes were only tested manually. I will add unit tests as soon as the general approach is approved!