-
Notifications
You must be signed in to change notification settings - Fork 523
bug: disable networking on devmode #2601
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
bug: disable networking on devmode #2601
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2601 +/- ##
==========================================
+ Coverage 46.92% 46.99% +0.06%
==========================================
Files 346 348 +2
Lines 55697 55715 +18
==========================================
+ Hits 26137 26183 +46
+ Misses 26611 26591 -20
+ Partials 2949 2941 -8
Continue to review full report at Codecov.
|
| cfg.DisableNetworking = true | ||
| } | ||
| node.config = cfg |
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.
| cfg.DisableNetworking = true | |
| } | |
| node.config = cfg | |
| node.config.DisableNetworking = true | |
| } |
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 was hoping to avoid setting this field twice - instead, I think that we should try and have a single copy of the config.Local. Until we do this, I think that ensuring that we "just" copy it here would make the next change slightly easier.
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.
Oops, I didn't realize cfg was used below as well.
Summary
The current code would not pass the
DisableNetworkingflag to the network package. This could be worked around by adding theDisableNetworkingto theconfig.jsonfile, or with this fix.Test Plan
Tested manually.