Rework configuration handling #205

ArieGato · 2025-03-29T10:02:18Z

Rework the configuration, so the configuration is seperated from cli program.

Add missing configurable properties

ini file:
Add http_port as option to the listen section of the ini file.

enviroment variables:
UPSTREAM for consistent naming. AMPQ_URL takes precendence over UPSTREAM for backwards compatibility cli

Cli options:
--log-level for the Log Level

resolves #204

Add --log-level switch Ignore missing ini file Add test cases

remove initial logging

add AMQP_URL to upstream setting

ArieGato · 2025-03-29T10:06:05Z

Are there any ci/test github actions that should be running?

dentarg · 2025-03-30T21:09:35Z

Yes but the workflow is missing the pull request event.

amqproxy/.github/workflows/ci.yml

Lines 1 to 11 in 4c9512d

    
           name: CI 
        
           on: 
        
             push: 
        
               paths: 
        
                 - 'run-specs-in-docker.sh' 
        
                 - '.github/workflows/ci.yml' 
        
                 - 'shard.yml' 
        
                 - 'shard.lock' 
        
                 - 'src/**' 
        
                 - 'spec/**' 
        
                 - 'test/**'

It should be corrected in main branch, and after that you can rebase, and CI will run.

ArieGato · 2025-03-31T06:18:26Z

so, add this?

  pull_request:
    branches:
      - main

snichme

How you handle Config and Options structs doesn't feel idiomatic crystal. You recreate the structs every time you call with instead of having it as one Class where you just update the fields on that object.

Having one object that you initialize in the CLI, keeping the reference through out the applications makes it easier to follow and any object that has a reference to that object will be able to read any changes to the config.

The content of the options.cr file looks like you repeat the same variables three times, this might be the way in C#. But in crystal you create one object and reuse that.

Breaking out the code form CLI.cr is nice, but keep it as one file, config and options can be merged.

ArieGato · 2025-04-02T13:45:11Z

How you handle Config and Options structs doesn't feel idiomatic crystal. You recreate the structs every time you call with instead of having it as one Class where you just update the fields on that object.
Having one object that you initialize in the CLI, keeping the reference through out the applications makes it easier to follow and any object that has a reference to that object will be able to read any changes to the config.

The general idea was to have an immutable object. Which can only be initialized through the load_with_cli method. Also, I think that config should not be altered after initialization.

I see what you mean about the remark about the Option.cr. Although I doubt Config and Option can be merged.

…o/amqproxy into feature/add-config-class

ArieGato · 2025-04-05T16:51:59Z

@snichme I did some rework. Could you take a look at the pr?

Also there are workflows failing, but I don't think it has anything to do with my changes.

snichme · 2025-04-07T09:19:48Z

The general idea was to have an immutable object. Which can only be initialized through the load_with_cli method. Also, I think that config should not be altered after initialization.

Any reason why it shouldn't be altered later on? If you have one config object and during runtime change that I think it's good that the change applies to all other objects that have a reference to the config object.

snichme · 2025-04-07T09:25:04Z

If you instead do it like this in cli.cr:

config = @config = AMQProxy::Config.new
  .load_from_file(options.ini_file || "config.ini")
  .load_from_env

and after than parse the options and set them directly on the config object there is no need to first create then options object and set all the options and them copy them over to the config object in the load_from_options method. It will also reduce the places one need to change if more configs are added.

Like so:

config = @config = AMQProxy::Config.new
  .load_from_file(options.ini_file || "config.ini")
  .load_from_env

parser.on("-p PORT", "--port=PORT", "Port to listen on (default: 5673)") { |v| config.listen_port = v.to_i }

wdyt?

ArieGato · 2025-04-07T10:33:02Z

From an OO perspective I believe that setting properties should be done from within the object. Other object should not need to know the correct precendence of loading the configuration. At the moment there are no validations on the configuration, but that should also be the responsibility of the config class. If combination of configuration properties are not valid, config can handle this.

If configuration is allowed to change in the lifecyle of the application, a method can be added for a specific action that results in configuration changes. e.g. a reload method on config that accepts new options.

I also considered adding the args as parameter to the config load_with_cli and parse the options there, but that didn't feel right.

snichme · 2025-04-07T10:44:04Z

From an OO perspective I believe that setting properties should be done from within the object. Other object should not need to know the correct precendence of loading the configuration. At the moment there are no validations on the configuration, but that should also be the responsibility of the config class. If combination of configuration properties are not valid, config can handle this.

This can still happen if you create a method like so: def listen_port=(value : UInt32) can you can validate the input and check if it's valid in combination with the current state.

ArieGato · 2025-04-07T12:59:38Z

@carlhoerberg The Build packages and Docker images workflows are failing for my PR. Is there something I'm msising here?

snichme · 2025-04-07T13:29:04Z

Here is another interesting idea on how to handle config values by using crystal macros, so you have one property and you can tag it with different annotations depending on if it's from file, env or as arg: cloudamqp/lavinmq#917

carlhoerberg · 2025-04-07T13:38:14Z

@carlhoerberg The Build packages and Docker images workflows are failing for my PR. Is there something I'm msising here?

That's expected and can be ignored

ArieGato · 2025-04-07T13:48:24Z

Here is another interesting idea on how to handle config values by using crystal macros, so you have one property and you can tag it with different annotations depending on if it's from file, env or as arg: cloudamqp/lavinmq#917

In that PR the cli arguments are parsed in the config class. If that is okay, then it can also solve the issue of the Options class.

I think there's one issue with the solution in that PR. In the ini file in amqproxy there are two settings that handle the same property. Or is it possible to add multiple attributes of the same type to a property?

when "bind", "address" then @listen_address = value

…n the Configu class

ArieGato · 2025-04-12T22:38:09Z

@snichme I moved the OptionParser to the config class and thus removing the extra Options record. Can you take another look at the PR?

snichme · 2025-04-17T09:48:09Z

src/amqproxy/config.cr

+    def self.load_with_cli(argv)
+      new()
+        .load_cli_options(argv.dup) # handle config file/help/version options
+        .load_from_file
+        .load_from_env
+        .load_cli_options(argv)
+    rescue ex
+      abort ex.message
+    end


Suggested change

def self.load_with_cli(argv)

new()

.load_cli_options(argv.dup) # handle config file/help/version options

.load_from_file

.load_from_env

.load_cli_options(argv)

rescue ex

abort ex.message

end

def initialize(argv)

load_cli_options(argv.dup)

load_from_file

load_from_env

load_cli_options(argv)

end

This feels more crystal style IMO, also move to top since it's the constructor

Would it be possible to do a solution where we didn't need to parse the ARGV twice?
Like the first load_cli_options returns something that we can later apply.

If we return a NamedTuple from load_cli_options we can also make use of that and pass the path to the config file to load_from_file like load_from_file(opts[:config_file]) to make it clearer.

When returning a NamedTuple we're sort of down to the Option record/class again. Which is fine by me, but is it a bad thing to process the cli options twice? Something that happens once at startup and seems cpu friendly.

snichme · 2025-04-17T09:51:42Z

src/amqproxy/config.cr

+            else                        raise "Unsupported config #{name}/#{key}"
+            end
+          end
+        else raise "Unsupported config section #{name}"


I know this exists today, but config options that isn't support shouldn't we just skip them?
How does it work for unsupported cli args, does that raise?

I agree that this seems a little harsh.

snichme · 2025-04-17T09:53:52Z

src/amqproxy/cli.cr

    end
+
+    unless config = @config
+      raise "Configuration has not been loaded"


This means that for some reason @config isn't set we dont disconnect clients, we just stop the process. Isn't @config always set here?

Yes It is always set, but here my Crystal knowledge falls short. I just don't know how I can convince the compiler it is. When I use not_nil I get the suggestion not to use this.

You need to make sure that it is set by being strict that it has a value, i.e. not letting it be nilable :)

snichme · 2025-04-17T09:54:56Z

src/amqproxy/cli.cr

-  @idle_connection_timeout : Int32 = 5
-  @term_timeout = -1
-  @term_client_close_timeout = 0
+  @config : AMQProxy::Config? = nil


Possible to make this not optional?

How can I do that? creating the instance happens in the run method.

Instead of a run() method, you can use an initializer and let cli.new() set @config when initializing, that way I think you can avoid letting it be Nilable.

ArieGato · 2025-04-22T12:30:06Z

@snichme Any thoughts on my replies?

kickster97 · 2025-04-29T07:19:19Z

spec/config_empty.ini

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.

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.

My reasoning is mostly about cleanliness so the repository remains free of files that are solely for transient test purposes.
Sure complexion grows slightly, thats a good point. I don't know the performance impact of creating an empty file, but is it really notably expensive to create once?

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.

Agree it should be more made more clear. Could be made by having a "fixtures" or "config fixtures" directory.

kickster97 · 2025-04-29T07:40:32Z

src/amqproxy/cli.cr

-  @idle_connection_timeout : Int32 = 5
-  @term_timeout = -1
-  @term_client_close_timeout = 0
+  @config : AMQProxy::Config? = nil


Instead of a run() method, you can use an initializer and let cli.new() set @config when initializing, that way I think you can avoid letting it be Nilable.

kickster97 · 2025-04-29T07:42:20Z

src/amqproxy/cli.cr

    end
+
+    unless config = @config
+      raise "Configuration has not been loaded"


You need to make sure that it is set by being strict that it has a value, i.e. not letting it be nilable :)

viktorerlingsson · 2025-04-29T08:27:45Z

spec/amqproxy/config_spec.cr

+    previous_argv = ARGV.clone
+    ARGV.clear
+
+    ARGV.concat([
+      "--config=/tmp/non_existing_file.ini",
+    ])
+
+    config = AMQProxy::Config.load_with_cli(ARGV)


You don't need to do all the ARGV handling in each spec, you can just pass an array of strings (or an empty array where needed) to load_with_cli instead.

Suggested change

previous_argv = ARGV.clone

ARGV.clear

ARGV.concat([

"--config=/tmp/non_existing_file.ini",

])

config = AMQProxy::Config.load_with_cli(ARGV)

config = AMQProxy::Config.load_with_cli(["--config=/tmp/non_existing_file.ini"])

viktorerlingsson · 2025-04-29T08:35:27Z

src/amqproxy/cli.cr

@@ -1,3 +1,4 @@
+require "./config"
 require "./version"


require "./version" should probably be moved to config.cr since that's where AMQProxy::VERSION is used?

carlhoerberg · 2025-06-11T06:52:46Z

amqproxy

What's this?

carlhoerberg · 2025-06-11T06:53:32Z

spec/Dockerfile

+COPY spec/config.ini /tmp/config.ini
+COPY spec/config_empty.ini /tmp/config_empty.ini


All files in spec/ is already copied on the line above?

ArieGato · 2025-06-11T13:50:48Z

Hi @carlhoerberg ,

To be honest, I'm a little hesitent to finish this PR.

I don't agree with several of the comments I got on this PR. Although my background is c#, some of the principles are not bound to a language. Things like immutability can result into more predictable code.

For instance the concept of a class to store the cli arguments in an Options class which is immutable is a solid idea. My initial implementation was not very in line with the syntax of Crystal. Although several of the properties match the ones of the eventual Config object shouldn't be an argument not to add it.

Another thing is exposing the methods for reading config sections cli/env/file/defaults is imo not the responsibility of the code that is invoking the methods. Just call Configure or some method name should suffice. It is the responsibility of the Config object to know the sequence.

Let me know what's your take on it.

Regards,
Arjan

ArieGato added 9 commits March 17, 2025 22:28

initial version of config class

ecb253e

Set load methods to protected

a2e55a8

Add --log-level switch Ignore missing ini file Add test cases

Add record example

f065f2d

Implement config record

16fbe1b

Fix config references

98b0d69

Merge remote-tracking branch 'origin/main' into feature/add-config-class

1d289ea

fix log_level handling and add specs

fc7e84c

remove initial logging

Add options record

224123a

move http_port to listen section

8a0187a

add AMQP_URL to upstream setting

ArieGato requested a review from a team as a code owner March 29, 2025 10:02

removed trailing space

43e26eb

Run ci on pull requests

09d8291

snichme reviewed Apr 1, 2025

View reviewed changes

ArieGato added 7 commits April 5, 2025 11:02

Rework Options as struct

71a05d1

change config to struct with properties

efe7f51

Merge branch 'feature/add-config-class' of https://github.com/ArieGat…

04c4e12

…o/amqproxy into feature/add-config-class

Merge from main

53f7008

Fix lint issues

a916871

run format

43b7f0c

fix boolean syntax

3d538b7

Merge branch 'main' into feature/add-config-class

8a29c1c

ArieGato added 4 commits April 12, 2025 21:51

Process review feedback. Removed option record, process cli options i…

933e310

…n the Configu class

tool format

98cda91

Remove options file

52c80e3

fix ameba suggestion

a919055

snichme reviewed Apr 17, 2025

View reviewed changes

kickster97 requested changes Apr 29, 2025

View reviewed changes

viktorerlingsson reviewed Apr 29, 2025

View reviewed changes

carlhoerberg reviewed Jun 11, 2025

View reviewed changes

		COPY spec/config.ini /tmp/config.ini
		COPY spec/config_empty.ini /tmp/config_empty.ini

Rework configuration handling #205

Are you sure you want to change the base?

Rework configuration handling #205

Uh oh!

Conversation

ArieGato commented Mar 29, 2025

Uh oh!

ArieGato commented Mar 29, 2025

Uh oh!

dentarg commented Mar 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ArieGato commented Mar 31, 2025

Uh oh!

snichme left a comment

Choose a reason for hiding this comment

Uh oh!

ArieGato commented Apr 2, 2025

Uh oh!

ArieGato commented Apr 5, 2025

Uh oh!

snichme commented Apr 7, 2025

Uh oh!

snichme commented Apr 7, 2025

Uh oh!

ArieGato commented Apr 7, 2025

Uh oh!

snichme commented Apr 7, 2025

Uh oh!

ArieGato commented Apr 7, 2025

Uh oh!

snichme commented Apr 7, 2025

Uh oh!

carlhoerberg commented Apr 7, 2025

Uh oh!

ArieGato commented Apr 7, 2025

Uh oh!

ArieGato commented Apr 12, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ArieGato commented Apr 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dentarg Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kickster97 Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

dentarg commented Mar 30, 2025 •

edited

Loading

dentarg Jun 11, 2025 •

edited

Loading

kickster97 Jun 11, 2025 •

edited

Loading