Skip to content

Conversation

@le-lenn
Copy link
Contributor

@le-lenn le-lenn commented Dec 21, 2025

Addresses #628

Approach

I looked into how shouterrr pretty-prints its verify function and tried to find the best poor-mans approach to it.

I wanted to add a little bit of formatting, because without any formatting, the output is pretty hard to read. So I landed on a basic regex based formatter.
I played around with coloring the output as well, but I ended up with a lot of code for a nice-to-have, so I removed it again.

@m90
Copy link
Member

m90 commented Dec 22, 2025

Just wanted to let you know I will need some more time in order to do a proper review here. I'm very happy the PR is here in any case.

@le-lenn
Copy link
Contributor Author

le-lenn commented Dec 22, 2025

No worries, frohe Weihnachten!

btw I’m not too happy with the flag parsing, I think my implementation could be improved

Copy link
Member

@m90 m90 left a comment

Choose a reason for hiding this comment

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

This looks good, I left some comments inline, let me know what you think.

There's one bigger question left for me in this: should there also be an option that "resolves" configuration values as deeply as possible, i.e. reads Docker secrets, expands shell variables, and renders config options that are templates? Or should this be the default even?

@le-lenn
Copy link
Contributor Author

le-lenn commented Jan 1, 2026

This looks good, I left some comments inline, let me know what you think.

There's one bigger question left for me in this: should there also be an option that "resolves" configuration values as deeply as possible, i.e. reads Docker secrets, expands shell variables, and renders config options that are templates? Or should this be the default even?

I was under the false impression that sourceConfiguration() would resolve all config values as deeply as possible already. Thanks for the hint! I think the default should be to resolve as deeply as possible, since the "resolved" values are the ones being used in the end.

I'll have a go to see if I can resolve without adding too much complexity.

@m90
Copy link
Member

m90 commented Jan 1, 2026

I was under the false impression that sourceConfiguration() would resolve all config values as deeply as possible already.

I would think it'd be a good thing to at least evaluate how much effort modeling things like this would be.

@le-lenn le-lenn changed the title Add "show-config" subcommand Add "print-config" subcommand Jan 1, 2026
@le-lenn
Copy link
Contributor Author

le-lenn commented Jan 1, 2026

I was under the false impression that sourceConfiguration() would resolve all config values as deeply as possible already.

I would think it'd be a good thing to at least evaluate how much effort modeling things like this would be.

I added some simple logic to resolve env vars when BackupFilenameExpand is set.

This is an AI analysis of what it would take to resolve all env variables:

  • Additional resolution happens later during runtime, not during sourcing:
    - applyEnv is only called in runScript, so conf.d‑provided env vars are not present for
    os.ExpandEnv in show-config (cmd/backup/run_script.go, cmd/backup/config.go).
    - Backup filename template ({{ .Extension }}), optional ExpandEnv, and strftime are applied in
    script.init (cmd/backup/script.go).
    - EMAIL_* → derived NotificationURLs and NotificationLevel validation also happen in
    script.init (cmd/backup/script.go).
    - Azure endpoint templates are resolved when the Azure backend is created (internal/storage/
    azure/azure.go).

    Complexity to “resolve more”:

    • Low/medium: extract a pure “resolve for display” helper to apply applyEnv, the backup filename
      template (extension + strftime using time.Now()), and ExpandEnv for the 3 fields. This is mostly
      moving logic out of script.init and would add some time‑dependent output.
    • Medium/high: include derived NotificationURLs and NotificationLevel validation. That requires
      sharing more of script.init or duplicating logic, and deciding how to surface warnings in show-
      config.
    • High: fully match runtime resolution (e.g., Azure endpoint template via backend constructors).
      That likely needs a refactor to split “config resolution” from “resource instantiation” in
      script.init to avoid side effects (sender setup, docker client, storage client creation).

@m90
Copy link
Member

m90 commented Jan 5, 2026

I had another look at the original issue: #628 which made me think we should check how the output handles trailing newlines here. Just writing this down so we don't forget.

@le-lenn
Copy link
Contributor Author

le-lenn commented Jan 10, 2026

I had another look at the original issue: #628 which made me think we should check how the output handles trailing newlines here. Just writing this down so we don't forget.

Do you have any ideas how to handle this?

@m90
Copy link
Member

m90 commented Jan 11, 2026

I had another look at the original issue: #628 which made me think we should check how the output handles trailing newlines here. Just writing this down so we don't forget.

Do you have any ideas how to handle this?

How does the current mechanism display a value with a trailing new line? I don't necessarily have an opinion on how exactly this should look like, but it should be obvious that it's really the configuration that contains the newline, not the print-config output.

formatter := regexp.MustCompile(`\s([A-Z])`)
for _, config := range configurations {
if err := func() error {
unset, warnings, err := config.resolve()
Copy link
Member

Choose a reason for hiding this comment

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

Is there any reason you don't use defer unset() here? Btw, it cannot be nil, so there's no need to check if it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, my go is a bit rusty! I appreciate all hints to improve my code

@le-lenn
Copy link
Contributor Author

le-lenn commented Jan 13, 2026

I had another look at the original issue: #628 which made me think we should check how the output handles trailing newlines here. Just writing this down so we don't forget.

Do you have any ideas how to handle this?

How does the current mechanism display a value with a trailing new line? I don't necessarily have an opinion on how exactly this should look like, but it should be obvious that it's really the configuration that contains the newline, not the print-config output.

This is how it currently looks if there is a trailing newline in a docker secrets file. The printed output also spans multiple lines.
Not super obvious to be honest, but is is there. We could also add a hint in the docs where it talks about docker secret files and recommend to check for trailing spaces or newlines.
CleanShot 2026-01-13 at 20 27 05@2x

Copy link
Member

@m90 m90 left a comment

Choose a reason for hiding this comment

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

This looks good now, thanks for adding this. I left one more comment regarding the docs. Once that's figured out I am happy to merge this and cut a new release 🎩

Copy link
Member

Choose a reason for hiding this comment

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

I think I'd feel safer if this doc still mentioned the output of print-config is not expected to be stable, i.e. people should not try to parse it and use it in scripts somehow. This way, it'd be possible to still tweak the formatting once we know how this is being used in the wild, and I would think the limitation is not too annoying for 99% of users.

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.

2 participants