Skip to content

Fix data_dir and config_file not reading systemd env vars#1844

Open
kickster97 wants to merge 2 commits intomainfrom
restore-systemd-env-vars
Open

Fix data_dir and config_file not reading systemd env vars#1844
kickster97 wants to merge 2 commits intomainfrom
restore-systemd-env-vars

Conversation

@kickster97
Copy link
Copy Markdown
Member

@kickster97 kickster97 commented Apr 7, 2026

WHAT is this pull request doing?

The config rewrite (#917) replaced the systemd-standard STATE_DIRECTORY and CONFIGURATION_DIRECTORY env vars with LAVINMQ_-prefixed variants. Since the systemd unit file (extras/lavinmq.service) declares StateDirectory=lavinmq and ConfigurationDirectory=lavinmq, systemd sets these vars automatically, and they are no longer read.

This means anyone using a non-default StateDirectory= path in a systemd override will have it silently ignored, falling back to the hardcoded /var/lib/lavinmq. Similarly, a custom ConfigurationDirectory= in a systemd override would be ignored when resolving the default config file path.

This PR restores both systemd env vars as fallbacks. LAVINMQ_DATADIR and LAVINMQ_CONFIGURATION_DIRECTORY still take precedence when set.

HOW can this pull request be tested?

run added spec

@kickster97 kickster97 requested a review from a team as a code owner April 7, 2026 08:24
@claude
Copy link
Copy Markdown

claude bot commented Apr 7, 2026

PR Review

No issues found.

The changes correctly implement systemd environment variable support:

  • STATE_DIRECTORY is read as a default for data_dir, with proper precedence: CLI > LAVINMQ_DATADIR env > ini > STATE_DIRECTORY > hardcoded default.
  • CONFIGURATION_DIRECTORY is used as a fallback for config directory lookup, with LAVINMQ_CONFIGURATION_DIRECTORY taking precedence.
  • Specs cover the new behavior and precedence rules.

@kickster97 kickster97 added this to the 2.7.0 milestone Apr 7, 2026
@[IniOpt(section: "main")]
@[EnvOpt("LAVINMQ_DATADIR")]
property data_dir : String = "/var/lib/lavinmq"
property data_dir : String = ENV.fetch("STATE_DIRECTORY", "/var/lib/lavinmq")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks to support multiple EnvOpt which means you could add
@[EnvOpt("STATE_DIRECTORY")] after the existing EnvOpt and it should pick one of them, the last should have precedence. Have you tested that?

Copy link
Copy Markdown
Member Author

@kickster97 kickster97 Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, yeah.. but the thing is that Crystals .annotation() (single) only returns the last annotation, so stacking @[EnvOpt] would silently drop all but the last env var. I'd need to change parse_env to use .annotations() (plural).

That works for data_dir, but config_file has to be resolved before parse_env runs because parse_ini needs the path first, so it still needs special handling. So I end up changing the macro, adding a transform lambda for the directory-to-filepath conversion, and still keeping the early lookup.

What do you think is the downside of the current solution?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's a tiny change to the code to support multiple EnvOpt.

With the proposed solution in this PR, won't ini config values take precedence over these envs?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the proposed solution in this PR, won't ini config values take precedence over these envs?

ah yes thats true, need to fix that!

@kickster97 kickster97 requested a review from snichme April 8, 2026 08:55
@kickster97 kickster97 requested a review from spuun April 9, 2026 08:20
@claude
Copy link
Copy Markdown

claude bot commented Apr 9, 2026

PR Review: No issues found.

The changes are clean and well-tested. Precedence chains are correct, the multi-annotation macro change is sound, and the removal of the buggy EnvOpt on config_file (which would have set a file path to a directory value) is a good fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants