-
Notifications
You must be signed in to change notification settings - Fork 743
[tmpnet] Switch back to using maps for subnet config #3877
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
Conversation
Using subnet.Config to define subnet configuration initially seemed like a good idea - typing for the win - but went down in flames due to a combination the semantics of JSON marshaling and how subnet config defaults are set by the node. So, back to maps we go.
7c40100
to
8eeca7f
Compare
Config: tmpnet.FlagsMap{ | ||
// Reducing this from the 1s default speeds up tx acceptance | ||
"proposerMinBlockDelay": 0, | ||
}, |
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.
Is this a no-op?
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.
It's not a no-op - the default is 1s.
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.
Why include a non no-op config change in this PR? Just looking to understand here
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.
This addition ensures tmpnet usage of subnet configuration for non-primary subnets.
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.
LGTM - couple of questions about whether or not some changes in tests are still no-ops in this PR
PR Chain: tmpnet+kube
This PR chain enables tmpnet to deploy temporary networks to Kubernetes. Early PRs refactor tmpnet to support the addition in #3615 of a new tmpnet node runtime for kube.
Why this should be merged
Using subnet.Config to define subnet configuration initially seemed like a good idea - typing for the win - but went down in flames due to a combination the semantics of JSON marshaling and how subnet config defaults are set by the node. So, back to maps we go.
How this was tested
CI
Need to be documented in RELEASES.md?
N/A