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

fix defaults not handled by yaml configuration #981

Merged
merged 2 commits into from
Sep 23, 2023
Merged

Conversation

crazy-max
Copy link
Owner

@crazy-max crazy-max commented Sep 23, 2023

related to #887

Current implementation of defaults is not working because the model.Image struct does not support proper yaml and json tags for the configuration.

Need a dedicated struct for this and also remove some fields that I don't think are necessary. Also move imageDefaults from watch to defaults in config root.

@crazy-max crazy-max force-pushed the fix-defaults branch 3 times, most recently from ae5da24 to e75f3bd Compare September 23, 2023 11:28
@crazy-max crazy-max marked this pull request as ready for review September 23, 2023 12:02
@crazy-max crazy-max merged commit 7c4b9d6 into master Sep 23, 2023
32 checks passed
@crazy-max crazy-max deleted the fix-defaults branch September 23, 2023 12:16
@IamTheFij
Copy link
Contributor

I like the idea of these being in their own section! I'm not sure I understand why it needs it's own struct though. It looks like the model.Image struct has field tags for yaml and json already. Using a new struct means a bit more overhead if new fields are added (probably unlikely, but still).

@crazy-max
Copy link
Owner Author

why it needs it's own struct though. It looks like the model.Image struct has field tags for yaml and json already. Using a new struct means a bit more overhead if new fields are added (probably unlikely, but still).

That's because the configuration loader does not support tags with special chars like _ that cannot be translated automatically to env vars like max_tags must be maxTags so env var would be DIUN_DEFAULTS_MAXTAGS.

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