-
Notifications
You must be signed in to change notification settings - Fork 186
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
Add CLI parameters for disable/enable peer #248
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.
Looking good, thanks for your contribution! Added just one supernit/question.
I'd wait a couple of days to that others also have an opportunity to review, but then this is good for merging from me.
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.
Overall this looks good, thanks for the PR! I think this is good to merge after following up on the clarifying questions.
Thanks for the quick review - I have been travelogn but will take a look and address the comments next week. |
Any chance for this being merged soon? |
Yes param only makes sense if name is provided.
Sorry for leaving this dangling for so long! I just lost of track of it. Updated as per the comments. |
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.
Looking great, thanks, @linuskendall! Good to merge, though would be good to test in real life first.
Want anyone in the community to quickly test? Ideally both the server and client variants of the new options.
Good to test. For reference - we have been running this patch for the last
6 months or so on our innernet instance!
(Deployed it and kind of forgot that it was a patch we added, so forgot
about this PR! Sorry!)
…On Wed, 13 Dec 2023 at 14:25, Matěj Laitl ***@***.***> wrote:
***@***.**** approved this pull request.
Looking great, thanks, @linuskendall <https://github.com/linuskendall>!
Good to merge, though would be good to test in real life first.
Want anyone in the community to quickly test? Ideally both the server and
client variants of the new options.
—
Reply to this email directly, view it on GitHub
<#248 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABHOYROX4RHY7452AZCR5F3YJFUPPAVCNFSM6AAAAAAVEWY426VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTONZZGEZTMMBYGI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
@strohel Taking you attention back to this. I think this could me deemed as tested. Can we merge? |
Thanks for ping, I've hit the merge button. Thanks everybody contributing ans sorry for the delays! |
Adds a --name parameter and --yes parameter to the enable-peer/disable-peer. Helps with large network management where we need some automated tools to disable/enable peers.
Fixes #214.