Skip to content

conf: support optional include? directive#7919

Open
olliehcrook wants to merge 1 commit intonats-io:mainfrom
olliehcrook:optional-includes
Open

conf: support optional include? directive#7919
olliehcrook wants to merge 1 commit intonats-io:mainfrom
olliehcrook:optional-includes

Conversation

@olliehcrook
Copy link
Copy Markdown

@olliehcrook olliehcrook commented Mar 7, 2026

Add parsing support for include? <path> so missing include files are ignored.

Keep existing behavior for invalid include files and add lexer/parser tests.

Implements: #5297

Signed-off-by: Ollie Hensman-Crook olliehensmancrook@gmail.com

@olliehcrook olliehcrook marked this pull request as ready for review March 7, 2026 19:32
@olliehcrook olliehcrook requested a review from a team as a code owner March 7, 2026 19:33
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 09a557f530

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Add parsing support for `include? <path>` so missing include files are ignored.

Keep existing behavior for invalid include files and add lexer/parser tests.

Implements: nats-io#5297
Signed-off-by: Ollie Hensman-Crook <olliehensmancrook@gmail.com>
@neilalexander
Copy link
Copy Markdown
Member

I'm trying to understand in what situation this would be useful. If an include is missing and we ignore it, then the system can and will come up in a possibly-unsupportable or unexpected state.

@wallyqs
Copy link
Copy Markdown
Member

wallyqs commented Mar 9, 2026

@neilalexander it is often requested as an alternative to create empty files or having to do it via external template systems, helps simplify that flow

@neilalexander
Copy link
Copy Markdown
Member

@wallyqs I am concerned mostly about the supportability aspect.

What happens in a situation like when you lose or accidentally truncate/delete/mangle an included file with the JetStream store_dir setting in it and it silently comes up with a temporary directory instead?

These kinds of hidden behaviours always end up being surprising at the worst times, usually during an incident, when you find out that the server isn't doing what you thought it was and it didn't fail-fast when it should have done.

@wallyqs
Copy link
Copy Markdown
Member

wallyqs commented Mar 9, 2026

@neilalexander the same thing could happen right now if using include and the included file becomes empty, this only skips when file is missing so parsing and other i/o errors aren't ignored either. One thing to detect the change too is to use the config digest to track whether the behavior of the past boot is different from the current boot, the presence of the include? would show it to be different.

@wallyqs
Copy link
Copy Markdown
Member

wallyqs commented Mar 9, 2026

I like the idea though of making it super explicit in terms of behavior in the config to have better protection, something like:

$ echo 'jetstream {
  store_dir = "./here"
}' > foo.conf

$ nats-server -c foo.conf -t
nats-server: configuration file foo.conf is valid (sha256:2e882ecf83c5d9ef4c4501f0f5bbe47ca57dcbe847d924cd69f26ee123da93f1)

then use:

include? ./foo.conf "sha256:2e882ecf83c5d9ef4c4501f0f5bbe47ca57dcbe847d924cd69f26ee123da93f1"

@neilalexander
Copy link
Copy Markdown
Member

@derekcollison Do you have any thoughts on this one?

@derekcollison
Copy link
Copy Markdown
Member

@derekcollison Do you have any thoughts on this one?

I am ok with this. Since its an opt-in via include? and current behavior is not affected.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants