Skip to content
This repository has been archived by the owner on Jan 8, 2024. It is now read-only.

Enabling dynamic config files #2416

Merged
merged 3 commits into from
Oct 6, 2021
Merged

Enabling dynamic config files #2416

merged 3 commits into from
Oct 6, 2021

Conversation

izaaklauer
Copy link
Contributor

Prior to this, it appears that dynamic config targeting files did not work - our logic was treating all dynamic config as env vars, and not respecting name_is_path .

This enables stanzas like this to work:

 config {
   file = {
    "/shared-config/nginx.conf" = configdynamic("vault", {
    path = "secret/data/apps/sample-app"
     key = "data/nginx.conf"
    })
   }
 }

I can't claim to have deeply understood all of the surrounding logic, so if there's a better place or way to do this, I'm open to feedback.

This also incidentally adds some better error messages to the vault config-sourcer plugin.

Prior to this, it appears that dynamic config targeting files did not work.

Also includes some better error messages for the vault plugin.
@izaaklauer izaaklauer marked this pull request as ready for review October 5, 2021 00:18
@izaaklauer izaaklauer requested a review from a team October 5, 2021 00:18
Copy link
Contributor

@mitchellh mitchellh left a comment

Choose a reason for hiding this comment

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

Only comment I’d have is that internal/appconfig is built to be relatively easy (or at least, possible) to test. It’s one of the design goals of the package and separating it from plugins and so on so we can just simulate any behavior. I’d recommend getting a failing test in here.

@izaaklauer izaaklauer requested a review from a team October 5, 2021 19:27
@izaaklauer izaaklauer merged commit a1bb7cc into main Oct 6, 2021
@izaaklauer izaaklauer deleted the dynamic-config-files branch October 6, 2021 12:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants