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.

Sign up for GitHub

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

Jump to bottom

feat: add a configuration service #416

Merged
merged 20 commits into from
Sep 6, 2024
Merged

feat: add a configuration service #416

merged 20 commits into from
Sep 6, 2024

Conversation

lengau
Copy link
Contributor

@lengau lengau commented Aug 13, 2024

  • Have you followed the guidelines for contributing?
  • Have you signed the CLA?
  • Have you successfully run tox?

Adds a service for handling application configuration. This service works as follows:

  1. Uses application-specific environment variables
  2. Uses general CRAFT_* environment variables
  3. Uses application-provided configuration handlers
  4. Uses snap configuration (if the app is installed as a snap)
  5. Falls back to the default or errors if no default is set.

@lengau lengau requested review from mr-cal and tigarmo August 14, 2024 12:38
@lengau lengau marked this pull request as ready for review August 14, 2024 12:38
@lengau lengau requested a review from a team August 15, 2024 17:35
@lengau lengau force-pushed the work/406/app-config branch from f013ae9 to 41a0b70 Compare August 15, 2024 17:55
@lengau
feat(config): limit use of CRAFT_* env vars
41a0b70 
CRAFT_* environment variables are now only used if the config item
is known to craft-application (not for app-specific config).
@lengau lengau mentioned this pull request Aug 15, 2024
Merged
4 tasks
@lengau lengau requested a review from a team August 23, 2024 17:37
mattculler
mattculler approved these changes Aug 26, 2024
View reviewed changes
Copy link
Contributor

@mattculler mattculler left a comment

Choose a reason for hiding this comment

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

Very slick. So I guess apps will subclass ConfigModel and can centralize all their envvars and those vars' default values.

craft_application/_config.py Show resolved Hide resolved
@lengau
Copy link
Contributor Author

lengau commented Aug 30, 2024

@tigarmo @cmatsuoka @dariuszd21 I'd like opinions on this from you before merging please :-)

dariuszd21
dariuszd21 approved these changes Sep 4, 2024
View reviewed changes
craft_application/application.py Outdated Show resolved Hide resolved
craft_application/application.py Outdated Show resolved Hide resolved
craft_application/services/config.py Show resolved Hide resolved
craft_application/services/provider.py Outdated Show resolved Hide resolved
tests/integration/test_application.py Show resolved Hide resolved
tests/conftest.py Show resolved Hide resolved
tests/unit/services/test_config.py Show resolved Hide resolved
tests/unit/services/test_config.py Show resolved Hide resolved
@dariuszd21 dariuszd21 requested a review from a team September 4, 2024 16:40
tigarmo
tigarmo approved these changes Sep 4, 2024
View reviewed changes
Copy link
Contributor

@tigarmo tigarmo left a comment

Choose a reason for hiding this comment

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

🚀

lengau and others added 6 commits September 5, 2024 22:55
@lengau @dariuszd21
chore: pr suggestions
90cd083 
Co-authored-by: Dariusz Duda <dariusz.duda@canonical.com>
@lengau
chore: minor updates
28e6f3f
@lengau
chore: add test
38ba2bb
@lengau
style: autoformat
a158d3a
@lengau @dariuszd21
chore: pr suggestion
853a99b 
Co-authored-by: Dariusz Duda <dariusz.duda@canonical.com>
@lengau
Merge branch 'main' into work/406/app-config
f5b3136
@lengau lengau merged commit 4d7bc64 into main Sep 6, 2024
7 checks passed
@lengau lengau deleted the work/406/app-config branch September 6, 2024 03:27
linostar pushed a commit to linostar/craft-application that referenced this pull request Dec 4, 2024
@lengau @dariuszd21
feat: add a configuration service (canonical#416)
cf58871 
Adds a configuration service that can be used to get application
configuration.

CRAFT_* environment variables are only used if the config item
is known to craft-application (not for app-specific config).

Co-authored-by: Dariusz Duda <dariusz.duda@canonical.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattculler mattculler mattculler approved these changes

@mr-cal mr-cal mr-cal approved these changes

@dariuszd21 dariuszd21 dariuszd21 approved these changes

@tigarmo tigarmo tigarmo approved these changes

@cmatsuoka cmatsuoka Awaiting requested review from cmatsuoka

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lengau @tigarmo @dariuszd21 @mr-cal @mattculler