-
Notifications
You must be signed in to change notification settings - Fork 52
Rework configuration handling #205
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
base: main
Are you sure you want to change the base?
Changes from 11 commits
ecb253e
a2e55a8
f065f2d
16fbe1b
98b0d69
1d289ea
fc7e84c
224123a
8a0187a
43e26eb
09d8291
71a05d1
efe7f51
04c4e12
53f7008
a916871
43b7f0c
3d538b7
8a29c1c
933e310
98cda91
52c80e3
a919055
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,8 @@ | ||
| name: CI | ||
| on: | ||
| pull_request: | ||
| branches: | ||
| - main | ||
| push: | ||
| paths: | ||
| - 'run-specs-in-docker.sh' | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,189 @@ | ||
| require "spec" | ||
| require "../../src/amqproxy/config" | ||
|
|
||
| describe AMQProxy::Config do | ||
| it "loads defaults when no ini file, env vars or options are available" do | ||
| options = AMQProxy::Options.new | ||
| options = options.with(ini_file: "/tmp/non_existing_file.ini") | ||
|
|
||
| config = AMQProxy::Config.load_with_cli(options) | ||
|
|
||
| config.listen_address.should eq "localhost" | ||
| config.listen_port.should eq 5673 | ||
| config.http_port.should eq 15673 | ||
| config.log_level.should eq ::Log::Severity::Info | ||
| config.idle_connection_timeout.should eq 5 | ||
| config.term_timeout.should eq -1 | ||
| config.term_client_close_timeout.should eq 0 | ||
| config.upstream.should eq nil | ||
| end | ||
|
|
||
| it "reads from empty config file returning default configuration" do | ||
| options = AMQProxy::Options.new | ||
| options = options.with(ini_file: "/tmp/config_empty.ini") | ||
|
|
||
| config = AMQProxy::Config.load_with_cli(options) | ||
|
|
||
| config.listen_address.should eq "localhost" | ||
| config.listen_port.should eq 5673 | ||
| config.http_port.should eq 15673 | ||
| config.log_level.should eq ::Log::Severity::Info | ||
| config.idle_connection_timeout.should eq 5 | ||
| config.term_timeout.should eq -1 | ||
| config.term_client_close_timeout.should eq 0 | ||
| config.upstream.should eq nil | ||
| end | ||
|
|
||
| it "reads from environment variables and use AMPQ_URL over UPSTREAM variable" do | ||
| options = AMQProxy::Options.new | ||
|
|
||
| ENV["AMQP_URL"] = "amqp://localhost:5673" | ||
| ENV["UPSTREAM"] = "amqp://localhost:5674" | ||
|
|
||
| config = AMQProxy::Config.load_with_cli(options) | ||
|
|
||
| config.upstream.should eq "amqp://localhost:5673" | ||
|
|
||
| # Clean up | ||
| ENV.delete("AMQP_URL") | ||
| ENV.delete("UPSTREAM") | ||
| end | ||
|
|
||
| it "reads from environment variables and overrules ini file values" do | ||
| options = AMQProxy::Options.new | ||
|
|
||
| ENV["LISTEN_ADDRESS"] = "example.com" | ||
| ENV["LISTEN_PORT"] = "5674" | ||
| ENV["HTTP_PORT"] = "15674" | ||
| ENV["LOG_LEVEL"] = "Error" | ||
| ENV["IDLE_CONNECTION_TIMEOUT"] = "12" | ||
| ENV["TERM_TIMEOUT"] = "13" | ||
| ENV["TERM_CLIENT_CLOSE_TIMEOUT"] = "14" | ||
| ENV["UPSTREAM"] = "amqp://localhost:5674" | ||
|
|
||
| config = AMQProxy::Config.load_with_cli(options) | ||
|
|
||
| config.listen_address.should eq "example.com" | ||
| config.listen_port.should eq 5674 | ||
| config.http_port.should eq 15674 | ||
| config.log_level.should eq ::Log::Severity::Error | ||
| config.idle_connection_timeout.should eq 12 | ||
| config.term_timeout.should eq 13 | ||
| config.term_client_close_timeout.should eq 14 | ||
| config.upstream.should eq "amqp://localhost:5674" | ||
|
|
||
| # Clean up | ||
| ENV.delete("LISTEN_ADDRESS") | ||
| ENV.delete("LISTEN_PORT") | ||
| ENV.delete("HTTP_PORT") | ||
| ENV.delete("LOG_LEVEL") | ||
| ENV.delete("IDLE_CONNECTION_TIMEOUT") | ||
| ENV.delete("TERM_TIMEOUT") | ||
| ENV.delete("TERM_CLIENT_CLOSE_TIMEOUT") | ||
| ENV.delete("UPSTREAM") | ||
| end | ||
|
|
||
| it "reads from command line arguments and overrules env vars" do | ||
| ENV["LISTEN_ADDRESS"] = "example.com" | ||
| ENV["LISTEN_PORT"] = "5674" | ||
| ENV["HTTP_PORT"] = "15674" | ||
| ENV["LOG_LEVEL"] = "Error" | ||
| ENV["IDLE_CONNECTION_TIMEOUT"] = "12" | ||
| ENV["TERM_TIMEOUT"] = "13" | ||
| ENV["TERM_CLIENT_CLOSE_TIMEOUT"] = "14" | ||
| ENV["UPSTREAM"] = "amqp://localhost:5674" | ||
|
|
||
| options = AMQProxy::Options.new | ||
| options = options.with( | ||
| listen_address: "example_arg.com", | ||
| listen_port: 5675, | ||
| http_port: 15675, | ||
| log_level: ::Log::Severity::Warn, | ||
| idle_connection_timeout: 15, | ||
| term_timeout: 16, | ||
| term_client_close_timeout: 17, | ||
| upstream: "amqp://localhost:5679" | ||
| ) | ||
|
|
||
| config = AMQProxy::Config.load_with_cli(options) | ||
|
|
||
| config.listen_address.should eq "example_arg.com" | ||
| config.log_level.should eq ::Log::Severity::Warn | ||
| config.listen_port.should eq 5675 | ||
| config.http_port.should eq 15675 | ||
| config.idle_connection_timeout.should eq 15 | ||
| config.term_timeout.should eq 16 | ||
| config.term_client_close_timeout.should eq 17 | ||
| config.upstream.should eq "amqp://localhost:5679" | ||
|
|
||
| # Clean Up | ||
| ENV.delete("LISTEN_ADDRESS") | ||
| ENV.delete("LISTEN_PORT") | ||
| ENV.delete("HTTP_PORT") | ||
| ENV.delete("LOG_LEVEL") | ||
| ENV.delete("IDLE_CONNECTION_TIMEOUT") | ||
| ENV.delete("TERM_TIMEOUT") | ||
| ENV.delete("TERM_CLIENT_CLOSE_TIMEOUT") | ||
| ENV.delete("UPSTREAM") | ||
| end | ||
|
|
||
| it "sets log level to debug when debug flag is present" do | ||
| options = AMQProxy::Options.new | ||
| options = options.with( | ||
| listen_address: "example_arg.com", | ||
| listen_port: 5675, | ||
| http_port: 15675, | ||
| log_level: ::Log::Severity::Warn, | ||
| idle_connection_timeout: 15, | ||
| term_timeout: 16, | ||
| term_client_close_timeout: 17, | ||
| is_debug: true, | ||
| upstream: "amqp://localhost:5679" | ||
| ) | ||
|
|
||
| config = AMQProxy::Config.load_with_cli(options) | ||
|
|
||
| config.listen_address.should eq "example_arg.com" | ||
| config.log_level.should eq ::Log::Severity::Debug | ||
| config.listen_port.should eq 5675 | ||
| config.http_port.should eq 15675 | ||
| config.idle_connection_timeout.should eq 15 | ||
| config.term_timeout.should eq 16 | ||
| config.term_client_close_timeout.should eq 17 | ||
| config.upstream.should eq "amqp://localhost:5679" | ||
| end | ||
|
|
||
| it "keeps the log level to trace when debug flag is present" do | ||
| options = AMQProxy::Options.new | ||
| options = options.with( | ||
| listen_address: "example_arg.com", | ||
| listen_port: 5675, | ||
| http_port: 15675, | ||
| log_level: ::Log::Severity::Trace, | ||
| idle_connection_timeout: 15, | ||
| term_timeout: 16, | ||
| term_client_close_timeout: 17, | ||
| is_debug: true, | ||
| upstream: "amqp://localhost:5679" | ||
| ) | ||
|
|
||
| config = AMQProxy::Config.load_with_cli(options) | ||
|
|
||
| config.log_level.should eq ::Log::Severity::Trace | ||
| end | ||
|
|
||
| it "reads default ini file when ini file path is null" do | ||
| options = AMQProxy::Options.new | ||
|
|
||
| config = AMQProxy::Config.load_with_cli(options) | ||
|
|
||
| config.listen_address.should eq "127.0.0.2" | ||
| config.listen_port.should eq 5678 | ||
| config.http_port.should eq 15678 | ||
| config.log_level.should eq ::Log::Severity::Debug | ||
| config.idle_connection_timeout.should eq 55 | ||
| config.term_timeout.should eq 56 | ||
| config.term_client_close_timeout.should eq 57 | ||
| config.upstream.should eq "amqp://localhost:5678" | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| require "spec" | ||
| require "../../src/amqproxy/options" | ||
|
|
||
| describe AMQProxy::Options do | ||
| it "can be changed by with yielding a new options record with correct property values" do | ||
| options = AMQProxy::Options.new | ||
|
|
||
| options = options.with( | ||
| listen_port: 34, | ||
| http_port: 35, | ||
| idle_connection_timeout: 36, | ||
| term_timeout: 37, | ||
| term_client_close_timeout: 38, | ||
| log_level: ::Log::Severity::Trace, | ||
| is_debug: true, | ||
| ini_file: "the_init_file.config", | ||
| listen_address: "listen.example.com", | ||
| upstream: "upstream.example.com:39" | ||
| ) | ||
|
|
||
| options.listen_address.should eq "listen.example.com" | ||
| options.listen_port.should eq 34 | ||
| options.upstream.should eq "upstream.example.com:39" | ||
| options.http_port.should eq 35 | ||
| options.idle_connection_timeout.should eq 36 | ||
| options.term_timeout.should eq 37 | ||
| options.term_client_close_timeout.should eq 38 | ||
| options.log_level.should eq ::Log::Severity::Trace | ||
| options.is_debug.should eq true | ||
| options.ini_file.should eq "the_init_file.config" | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| [main] | ||
| log_level = debug | ||
| idle_connection_timeout = 55 | ||
| term_timeout = 56 | ||
| term_client_close_timeout = 57 | ||
| upstream = amqp://localhost:5678 | ||
|
|
||
| [listen] | ||
| bind = 127.0.0.1 | ||
| address = 127.0.0.2 | ||
| port = 5678 | ||
| http_port = 15678 | ||
| log_level = debug |
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would be good to avoid having empty files in the repository, maybe this file can be created when needed in config_spec.cr and then cleaned up at the end of the spec. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is it good to avoid empty files in the repo? If the file serves a purpose (used in specs? I haven't actually checked this 🙈), we should not be afraid to have it around. Creating it with code sounds both more complex and slower to me. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My reasoning is mostly about cleanliness so the repository remains free of files that are solely for transient test purposes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we have files like this in the repo I think it should be clear in some way what they are used for so its less confusing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree it should be more made more clear. Could be made by having a "fixtures" or "config fixtures" directory. |
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.
All files in
spec/is already copied on the line above?