Skip to content

Conversation

@tsachiherman
Copy link
Contributor

Summary

The current code would not pass the DisableNetworking flag to the network package. This could be worked around by adding the DisableNetworking to the config.json file, or with this fix.

Test Plan

Tested manually.

@codecov-commenter
Copy link

codecov-commenter commented Jul 20, 2021

Codecov Report

Merging #2601 (93cfca7) into master (eeee334) will increase coverage by 0.06%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
node/node.go 3.78% <100.00%> (ø)
ledger/roundlru.go 90.56% <0.00%> (-5.67%) ⬇️
ledger/acctupdates.go 61.83% <0.00%> (-0.50%) ⬇️
util/db/fullfsync_darwin.go 100.00% <0.00%> (ø)
libgoal/lockedFileUnix.go 0.00% <0.00%> (ø)
ledger/eval.go 76.68% <0.00%> (+0.29%) ⬆️
network/wsNetwork.go 61.11% <0.00%> (+0.37%) ⬆️
util/db/dbutil.go 39.54% <0.00%> (+0.56%) ⬆️
catchup/service.go 69.79% <0.00%> (+0.78%) ⬆️
catchup/peerSelector.go 100.00% <0.00%> (+1.04%) ⬆️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eeee334...93cfca7. Read the comment docs.

Comment on lines 170 to +172
cfg.DisableNetworking = true
}
node.config = cfg
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
cfg.DisableNetworking = true
}
node.config = cfg
node.config.DisableNetworking = true
}

Copy link
Contributor Author

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.

Copy link
Contributor

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.

@tsachiherman tsachiherman merged commit 9bcee44 into algorand:master Jul 21, 2021
@tsachiherman tsachiherman deleted the tsachi/fixdevmodenetworking branch July 21, 2021 19:04
@tsachiherman tsachiherman restored the tsachi/fixdevmodenetworking branch August 16, 2021 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants