Skip to content

goal: allow ConfigJSONOverride in local network templates#5017

Merged
algorandskiy merged 4 commits into
algorand:masterfrom
algorandskiy:pavel/goal-net
Feb 1, 2023
Merged

goal: allow ConfigJSONOverride in local network templates#5017
algorandskiy merged 4 commits into
algorand:masterfrom
algorandskiy:pavel/goal-net

Conversation

@algorandskiy
Copy link
Copy Markdown
Contributor

@algorandskiy algorandskiy commented Jan 13, 2023

Summary

Allow ConfigJSONOverride in goal network create templates similarly to netgoal.

Test Plan

Tested manually

@algorandskiy algorandskiy self-assigned this Jan 13, 2023
@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 13, 2023 •
edited
Loading

Codecov Report

Merging #5017 (10a1e75) into master (5d38a15) will increase coverage by 0.00%.
The diff coverage is 0.00%.

@@           Coverage Diff           @@
##           master    #5017   +/-   ##
=======================================
  Coverage   53.62%   53.63%           
=======================================
  Files         432      432           
  Lines       54060    54065    +5     
=======================================
+ Hits        28991    28999    +8     
+ Misses      22824    22821    -3     
  Partials     2245     2245
Impacted Files Coverage Δ
netdeploy/networkTemplate.go 28.33% <0.00%> (-1.24%) ⬇️
netdeploy/remote/nodeConfig.go 0.00% <ø> (ø)
catchup/peerSelector.go 98.95% <0.00%> (-1.05%) ⬇️
network/wsNetwork.go 65.01% <0.00%> (+0.08%) ⬆️
catchup/service.go 69.08% <0.00%> (+0.48%) ⬆️
ledger/tracker.go 75.10% <0.00%> (+0.84%) ⬆️
ledger/blockqueue.go 88.50% <0.00%> (+2.87%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

brianolson
brianolson reviewed Jan 19, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@brianolson brianolson left a comment

Choose a reason for hiding this comment

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

Go code looks fine, I wrote a unit test that I sent a PR about to add to this.
What's the implication of the mkconf.py change?

Comment thread test/testdata/nettemplates/perf/mkconf.py
"Name": "NPNode%d" % n,
"Wallets": [],
"DeadlockDetection": -1,
"ConfigJSONOverride": '{"ForceFetchTransactions":true}'
Copy link
Copy Markdown
Contributor

@brianolson brianolson Jan 19, 2023

Choose a reason for hiding this comment

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

This is what the change is really about. So, why this?

Copy link
Copy Markdown
Contributor Author

@algorandskiy algorandskiy Jan 19, 2023

Choose a reason for hiding this comment

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

I used mkconf to generate local topologies for some tests, and decided to leave ConfigJSONOverride example there. I can remove if it does not make much sense having it here.

@algorandskiy algorandskiy requested review from algonautshant, cce and winder and removed request for algobolson and algocce January 30, 2023 22:21
@winder
Copy link
Copy Markdown
Contributor

winder commented Jan 30, 2023

Nice. This is semi related to a "profile" feature we've been working on #5069

@winder winder requested a review from mciccarello January 30, 2023 22:33
@brianolson
Copy link
Copy Markdown
Contributor

brianolson commented Jan 31, 2023

I think the mkconf.py change is a little weird as an "example" because it has concrete consequences that aren't necessarily intended?
How about this additional test to ensure that encoding/json does overlay behavior correctly? algorandskiy#6

@algorandskiy
Copy link
Copy Markdown
Contributor Author

algorandskiy commented Jan 31, 2023 •
edited
Loading

fair, thank you, applied.
I also set number of npn to 0 in mkconf.py to keep the result the same but also demonstrating ConfigJSONOverride usage.

brianolson
brianolson previously approved these changes Feb 1, 2023
View reviewed changes
algonautshant
algonautshant reviewed Feb 1, 2023
View reviewed changes
Comment thread netdeploy/networkTemplates_test.go
@algorandskiy @algonautshant
Update netdeploy/networkTemplates_test.go
10a1e75 
Co-authored-by: Shant Karakashian <55754073+algonautshant@users.noreply.github.com>
algonautshant
algonautshant approved these changes Feb 1, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@algonautshant algonautshant left a comment

Choose a reason for hiding this comment

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

Looks fine. Very useful.

Comment thread netdeploy/networkTemplates_test.go
}

// TestJsonOverlay ensures that encoding/json will only clobber fields present in the json and leave other fields unchanged
func TestJsonOverlay(t *testing.T) {
Copy link
Copy Markdown
Contributor

@algonautshant algonautshant Feb 1, 2023

Choose a reason for hiding this comment

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

What is the purpose of this test? It looks like it is a json and dec test, and not a netdeploy test.
Ideally, the test should test netdeploy/.

@algorandskiy algorandskiy merged commit ee593c1 into algorand:master Feb 1, 2023
@onetechnical onetechnical mentioned this pull request Mar 1, 2023
Merged
@Algo-devops-service Algo-devops-service mentioned this pull request Mar 10, 2023
Closed
This was referenced Mar 10, 2023
Merged
Merged
@algorandskiy algorandskiy deleted the pavel/goal-net branch March 16, 2026 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cce cce Awaiting requested review from cce

@mciccarello mciccarello Awaiting requested review from mciccarello

3 more reviewers

@winder winder winder approved these changes

@algonautshant algonautshant algonautshant approved these changes

@brianolson brianolson brianolson left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@algorandskiy algorandskiy

Labels

Enhancement Team Carbon-11

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@algorandskiy @winder @brianolson @algonautshant