-
Notifications
You must be signed in to change notification settings - Fork 116
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
common/config: move internal UNIX socket path option to config file #5427
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
config: add option to override internal unix socket path | ||
|
||
Previously the UNIX socket path could only be overriden via a debug option | ||
which also required the general "don't blame Oasis" to be set. Since this | ||
option can be generally useful in production environments it is now supported | ||
in the config file. The socket path can be set under | ||
`common.internal_socket_path`, and is not considered a debug option anymore. |
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
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
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
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -491,6 +491,45 @@ consensus: | |
- "[email protected]:26656" | ||
` | ||
|
||
// Simple config with internal socket override. | ||
const testInternalSocketOverrideConfigRaw = ` | ||
debug: | ||
dont_blame_oasis: true | ||
grpc: | ||
internal: | ||
socket_path: /node/custom-internal.sock | ||
|
||
datadir: /node/data | ||
|
||
log: | ||
level: | ||
default: debug | ||
tendermint: debug | ||
tendermint/context: error | ||
format: JSON | ||
|
||
genesis: | ||
file: /node/etc/genesis.json | ||
|
||
consensus: | ||
tendermint: | ||
p2p: | ||
seed: | ||
- "[email protected]:26656" | ||
|
||
runtime: | ||
mode: client | ||
paths: | ||
- /node/runtime/cipher-paratime-2.6.2.orc | ||
- /node/runtime/emerald-paratime-10.0.0.orc | ||
- /node/runtime/sapphire-paratime-0.4.0.orc | ||
|
||
worker: | ||
storage: | ||
checkpointer: | ||
enabled: true | ||
` | ||
|
||
func prepareTest(require *require.Assertions, configIn string) config.Config { | ||
// Prepare temporary directory and populate it with the test config file. | ||
tempDir, err := os.MkdirTemp("", "oasis-node-config_migrate_test_") | ||
|
@@ -778,3 +817,24 @@ func TestConfigMigrationDocsSentry(t *testing.T) { | |
require.Equal(newConfig.P2P.Seeds[1], "H6u9MtuoWRKn5DKSgarj/[email protected]:9200") | ||
require.Equal(newConfig.Consensus.SentryUpstreamAddresses[0], "[email protected]:26656") | ||
} | ||
|
||
func TestConfigMigrationSocketOverride(t *testing.T) { | ||
require := require.New(t) | ||
newConfig := prepareTest(require, testInternalSocketOverrideConfigRaw) | ||
|
||
// Now check if the config struct fields actually match the original state. | ||
require.Equal(newConfig.Mode, config.ModeClient) | ||
require.Equal(newConfig.Common.DataDir, "/node/data") | ||
require.Equal(newConfig.Common.Log.Format, "JSON") | ||
require.Equal(newConfig.Common.Log.Level["default"], "debug") | ||
require.Equal(newConfig.Common.Log.Level["cometbft"], "debug") | ||
require.Equal(newConfig.Common.Log.Level["cometbft/context"], "error") | ||
require.Equal(newConfig.Genesis.File, "/node/etc/genesis.json") | ||
require.Equal(newConfig.P2P.Seeds[0], "H6u9MtuoWRKn5DKSgarj/[email protected]:26656") | ||
require.Equal(newConfig.P2P.Seeds[1], "H6u9MtuoWRKn5DKSgarj/[email protected]:9200") | ||
require.Equal(newConfig.Runtime.Paths[0], "/node/runtime/cipher-paratime-2.6.2.orc") | ||
require.Equal(newConfig.Runtime.Paths[1], "/node/runtime/emerald-paratime-10.0.0.orc") | ||
require.Equal(newConfig.Runtime.Paths[2], "/node/runtime/sapphire-paratime-0.4.0.orc") | ||
require.Equal(newConfig.Storage.Checkpointer.Enabled, true) | ||
require.Equal(newConfig.Common.InternalSocketPath, "/node/custom-internal.sock") | ||
} |
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
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
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
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
Oops, something went wrong.
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.
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.
Can this also be removed?
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.
Yeah I'll look into removing this in a follow up as this was already merged. I don't see a reason why the datadir was required here specifically - even before the change in this PR.
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.
To construct the path, I guess.
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.
Yeah but this didn't change with the PR. It still uses the same data dir config value to construct it (unless overriden).