Skip to content
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

add support for UCL ".include" macro #15

Merged
merged 1 commit into from
May 16, 2023
Merged

Conversation

fraenki
Copy link
Member

@fraenki fraenki commented Mar 27, 2023

This PR adds support for the UCL ".include" macro.

These hiera options...

rspamd::config:
  classifier-bayes:
    .include(try=true,priority=1,duplicate=merge): $LOCAL_CONFDIR/local.d/section.conf

...will result in the following rspamd configuration:

.include(try=true,priority=1,duplicate=merge) "$LOCAL_CONFDIR/local.d/section.conf"

manifests/ucl/config.pp Outdated Show resolved Hide resolved
@oxc
Copy link
Collaborator

oxc commented Mar 28, 2023

Will it work properly for multiple includes? Are they usually all different on the left hand side?

@fraenki
Copy link
Member Author

fraenki commented Mar 28, 2023

Will it work properly for multiple includes? Are they usually all different on the left hand side?

It will work as long as the left side is different (at least in Hiera). I think it will always be feasible to use a different priority to make it work:

rspamd::config:
  classifier-bayes:
    .include(try=true,priority=1,duplicate=merge): $LOCAL_CONFDIR/local.d/section.conf
    .include(try=true,priority=5,duplicate=merge): $LOCAL_CONFDIR/local.d/section5.conf

I'd favor full support for UCL macros, but I don't think there is enough interest – and this small addition gets the job done (for me). :)

@oxc
Copy link
Collaborator

oxc commented Mar 28, 2023

Can I, out of curiosity, ask what your use case for this is?

manifests/ucl/config.pp Outdated Show resolved Hide resolved
@fraenki
Copy link
Member Author

fraenki commented Mar 28, 2023

Can I, out of curiosity, ask what your use case for this is?

I need to include multiple rspamd "plugins". They provide their own config files, e.g. for rbl.conf and rbl_group.conf. And I want to use their unmodified files and still be able to provide my own rbl.conf. So I use the .include macro to include all 3rd-party files.

@oxc
Copy link
Collaborator

oxc commented Mar 28, 2023

I kind of would prefer a syntax like this:

rspamd::config:
  classifier-bayes:
    .include:
      - args:
          try: true
          priority: 1
          duplicate: merge:
        value: "$LOCAL_CONFDIR/local.d/section.conf"
      - args:
          try: true
          priority: 2
          duplicate: merge:
        value: "$LOCAL_CONFDIR/local.d/section5.conf"

Should not be that hard to implement, I believe. What do you think?

@fraenki
Copy link
Member Author

fraenki commented Mar 28, 2023

This sure looks much better. 👍 I don't like my ugly quick'n'dirty effort, but I don't have enough time to work on a more sophisticated solution. :-/

@oxc
Copy link
Collaborator

oxc commented Mar 28, 2023

I know. That's why I'm inclined to approve the PR 😂 Give me a couple of days to see what I can do, otherwise I will merge your changes.

@oxc
Copy link
Collaborator

oxc commented Mar 28, 2023

My proposed syntax should be compatible with/distinguishable from yours, so we can also just add it later 🙃

@fraenki
Copy link
Member Author

fraenki commented Mar 28, 2023

I have no objections 😁

@fraenki fraenki self-assigned this May 16, 2023
@fraenki fraenki merged commit e3aef20 into markt-de:master May 16, 2023
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.

None yet

2 participants