[Fix] Make --rest optional to prevent auto-change of REST port with --dev #3221
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
christianwwwwwwww
changed the title
christianwwwwwwww
changed the title
christianwwwwwwww
changed the title
iamalwaysuncomfortable
requested changes
Apr 18, 2024
iamalwaysuncomfortable
requested changes
Apr 18, 2024
cli/src/commands/start.rs
Outdated
Comment on lines
839
to
855
| fn test_rest_ip_behavior() { | ||
| // Test default REST IP when not specified in dev mode | ||
| let config = Start::try_parse_from(["snarkos", "--dev", "1"].iter()).unwrap(); | ||
| assert_eq!(config.rest, Some(SocketAddr::from_str("0.0.0.0:3030").unwrap())); | ||
|
|
||
| // Test specified REST IP when passed in dev mode | ||
| let config = Start::try_parse_from(["snarkos", "--dev", "1", "--rest", "127.0.0.1:8080"].iter()).unwrap(); | ||
| assert_eq!(config.rest, Some(SocketAddr::from_str("127.0.0.1:8080").unwrap())); | ||
|
|
||
| // Test default REST IP when REST flag is not passed in prod mode | ||
| let config = Start::try_parse_from(["snarkos"].iter()).unwrap(); | ||
| assert_eq!(config.rest, Some(SocketAddr::from_str("0.0.0.0:3030").unwrap())); | ||
|
|
||
| // Test specified REST IP when passed in prod mode | ||
| let config = Start::try_parse_from(["snarkos", "--rest", "192.168.1.1:8080"].iter()).unwrap(); | ||
| assert_eq!(config.rest, Some(SocketAddr::from_str("192.168.1.1:8080").unwrap())); | ||
| } |
There was a problem hiding this comment.
This is a great start, can you write a few more test cases covering more potential cases?
|
I'm not sure of the scope of this change, but no matter what, I want to make sure that anytime we require a port that we can set both host:port to a specific values. There's a tendency for us to simply bind to 0.0.0.0:port and this breaks multi-homed deployments in production environments. Dev environments, I understand, are special, but for everything else, these values, at a minimum, should be configurable either via command line or in a config file. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Solves Issue #3220 wherein specified rest port was overwritten by the default when the
--devflag was being used.Test Plan
This will only change a node's behavior when both
--devand--restare used.Tested locally.
Related PRs
(Link any related PRs here)