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

feat: Convert config template to pydantic model #1364

Merged
merged 1 commit into from
Mar 22, 2024

Conversation

romeonicholas
Copy link
Contributor

@romeonicholas romeonicholas commented Feb 22, 2024

Resolves #1165 by replacing the configuration template with a pydantic object from which default configurations can be written.

  • Attempting to launch the app without a configuration file will now automatically generate a configuration
  • The application loads the configuration as a pydantic object, and all existing references to the YAML have been switched over to using the python object attributes instead
  • The new model uses snake case naming conventions for attributes, with camel case aliases for writing to YAML (this is done automatically for all attributes, but a few require manual aliasing for some common acronyms)

Copy link

codecov bot commented Feb 22, 2024

Codecov Report

Attention: Patch coverage is 85.39326% with 26 lines in your changes are missing coverage. Please review.

Project coverage is 75.86%. Comparing base (d6decd0) to head (66728d2).

Files Patch % Lines
...acollab/core/authentication/provider/oauth/flow.py 40.00% 5 Missing and 1 partial ⚠️
...lab/core/authentication/provider/azure/keystore.py 0.00% 4 Missing ⚠️
...lab/core/authentication/provider/azure/__main__.py 0.00% 3 Missing ⚠️
...ollab/core/authentication/provider/azure/routes.py 0.00% 3 Missing ⚠️
backend/capellacollab/sessions/operators/k8s.py 72.72% 1 Missing and 2 partials ⚠️
backend/capellacollab/__main__.py 0.00% 1 Missing ⚠️
backend/capellacollab/config/__init__.py 75.00% 0 Missing and 1 partial ⚠️
...nd/capellacollab/core/authentication/jwt_bearer.py 0.00% 1 Missing ⚠️
...lab/core/authentication/provider/oauth/keystore.py 80.00% 1 Missing ⚠️
backend/capellacollab/core/database/migration.py 50.00% 1 Missing ⚠️
... and 2 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1364      +/-   ##
==========================================
+ Coverage   75.03%   75.86%   +0.83%     
==========================================
  Files         169      170       +1     
  Lines        5675     5738      +63     
  Branches      659      659              
==========================================
+ Hits         4258     4353      +95     
+ Misses       1268     1238      -30     
+ Partials      149      147       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@romeonicholas romeonicholas force-pushed the pydantic-config-template branch from ab33df1 to 94f6d19 Compare February 27, 2024 07:06
@romeonicholas romeonicholas force-pushed the pydantic-config-template branch from 94f6d19 to 93eade4 Compare February 27, 2024 07:14
@romeonicholas
Copy link
Contributor Author

romeonicholas commented Feb 27, 2024

I'm definitely doing something wrong here but I'm not familiar with yaml.
@MoritzWeber0 my yaml output is currently looking like this, do you think I'm doing something wrong in my classes or in deriving the yaml from the classes?:
(the function generating the yaml is just at the bottom of config_template.py for now)

docker: !!python/name:__main__.DockerConfig ''
k8s: !!python/name:__main__.K8sConfig ''
general: !!python/name:__main__.GeneralConfig ''
extensions: !!python/name:__main__.ExtensionsConfig ''
...
...

@MoritzWeber0
Copy link
Member

I'm definitely doing something wrong here but I'm not familiar with yaml. @MoritzWeber0 my yaml output is currently looking like this, do you think I'm doing something wrong in my classes or in deriving the yaml from the classes?: (the function generating the yaml is just at the bottom of config_template.py for now)

docker: !!python/name:__main__.DockerConfig ''
k8s: !!python/name:__main__.K8sConfig ''
general: !!python/name:__main__.GeneralConfig ''
extensions: !!python/name:__main__.ExtensionsConfig ''
...
...

Use app_config.model_dump() instead app_config.__dict__

@romeonicholas
Copy link
Contributor Author

Perfect, thanks @MoritzWeber0, I switched to BaseModels so I could use model_dump() and now it's almost correct, just need to tweak a few fields I entered incorrectly. Last question would be: how do we present or implement this for the user?

  • Easiest to implement but most work for the end user: just update the documentation to say something like "you can run the following command to generate your config before first launch" as part of the setup process
  • Hardest to implement but easiest for the user: automatically generate a config based off the template if they don't have one (where would that happen? The Makefile?)

Option A would obviously only take me a few minutes, I'd need a little starting direction for Option B so I knew where to look.

@MoritzWeber0
Copy link
Member

Perfect, thanks @MoritzWeber0, I switched to BaseModels so I could use model_dump() and now it's almost correct, just need to tweak a few fields I entered incorrectly. Last question would be: how do we present or implement this for the user?

  • Easiest to implement but most work for the end user: just update the documentation to say something like "you can run the following command to generate your config before first launch" as part of the setup process
  • Hardest to implement but easiest for the user: automatically generate a config based off the template if they don't have one (where would that happen? The Makefile?)

Option A would obviously only take me a few minutes, I'd need a little starting direction for Option B so I knew where to look.

Go for option A:

  • If no config file exists in the list of places, fail during startup. Log an error message how to generate the config file.
  • Add a CLI option (we already have an CLI interface for config.diff) to generate the config file based on the default values.

backend/config/config_template.py Outdated Show resolved Hide resolved
backend/.gitignore Outdated Show resolved Hide resolved
backend/config/config_template.py Outdated Show resolved Hide resolved
backend/config/config_template.py Outdated Show resolved Hide resolved
@romeonicholas romeonicholas force-pushed the pydantic-config-template branch from 93eade4 to b4305b9 Compare February 27, 2024 13:22
@romeonicholas romeonicholas force-pushed the pydantic-config-template branch 5 times, most recently from 411e162 to 8f34a29 Compare February 28, 2024 15:06
@romeonicholas romeonicholas force-pushed the pydantic-config-template branch 4 times, most recently from 19c20ac to ae531cc Compare February 28, 2024 16:38
@romeonicholas romeonicholas marked this pull request as ready for review February 28, 2024 16:42
@romeonicholas romeonicholas force-pushed the pydantic-config-template branch 7 times, most recently from c3168bd to dccc22e Compare March 5, 2024 10:05
@romeonicholas romeonicholas force-pushed the pydantic-config-template branch 2 times, most recently from 3ac7c9d to f51aa91 Compare March 18, 2024 16:27
@romeonicholas romeonicholas marked this pull request as draft March 19, 2024 10:59
@romeonicholas romeonicholas force-pushed the pydantic-config-template branch 9 times, most recently from e3aeb12 to 3530e49 Compare March 21, 2024 10:15
url: str = pydantic.Field(
default="postgresql://dev:dev@localhost:5432/dev",
description="The URL of the database (format: postgresql://[userspec@][hostspec][/dbname][?paramspec]).",
examples=["postgresql://dev:dev@localhost:5432/dev"],

Check failure

Code scanning / SonarCloud

PostgreSQL database passwords should not be disclosed

<!--SONAR_ISSUE_KEY:AY5ggtmwoBno06kVFNY_-->Make sure this PostgreSQL database password gets changed and removed from the code. <p>See more on <a href="https://sonarcloud.io/project/issues?id=DSD-DBS_capella-collab-manager&issues=AY5ggtmwoBno06kVFNY_&open=AY5ggtmwoBno06kVFNY_&pullRequest=1364">SonarCloud</a></p>

class DatabaseConfig(BaseConfig):
url: str = pydantic.Field(
default="postgresql://dev:dev@localhost:5432/dev",

Check failure

Code scanning / SonarCloud

PostgreSQL database passwords should not be disclosed

<!--SONAR_ISSUE_KEY:AY5ggtmwoBno06kVFNY--->Make sure this PostgreSQL database password gets changed and removed from the code. <p>See more on <a href="https://sonarcloud.io/project/issues?id=DSD-DBS_capella-collab-manager&issues=AY5ggtmwoBno06kVFNY-&open=AY5ggtmwoBno06kVFNY-&pullRequest=1364">SonarCloud</a></p>
@romeonicholas romeonicholas marked this pull request as ready for review March 21, 2024 10:38
@romeonicholas romeonicholas marked this pull request as draft March 21, 2024 11:08
@romeonicholas romeonicholas force-pushed the pydantic-config-template branch 2 times, most recently from 82c125a to 0f08c64 Compare March 21, 2024 15:25
@romeonicholas romeonicholas marked this pull request as ready for review March 21, 2024 15:31
@DSD-DBS DSD-DBS deleted a comment from github-actions bot Mar 21, 2024
@DSD-DBS DSD-DBS deleted a comment from github-actions bot Mar 21, 2024
Copy link
Member

@MoritzWeber0 MoritzWeber0 left a comment

Choose a reason for hiding this comment

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

Thanks @romeonicholas. This is one of the best PRs of this year and will significantly improve my life. Finally, we have typing of the configuration.

I love the precision of changes and the accurate descriptions and examples. I did some changes and added conversations for it, but everything is already resolved. It's just for transparency what I've changed.

Tested on staging and works like a dream.

I'd just refactor the tests slightly: Instead of using unittest mock, I'd switch to Python classes which have typing and autocomplete.

backend/capellacollab/config/__init__.py Show resolved Hide resolved
backend/capellacollab/config/diff.py Outdated Show resolved Hide resolved
backend/capellacollab/config/models.py Show resolved Hide resolved
backend/capellacollab/config/models.py Show resolved Hide resolved
backend/capellacollab/config/models.py Outdated Show resolved Hide resolved
backend/capellacollab/sessions/hooks/http.py Outdated Show resolved Hide resolved
backend/capellacollab/sessions/util.py Outdated Show resolved Hide resolved
backend/tests/config/test_configuration.py Outdated Show resolved Hide resolved
backend/tests/config/test_configuration.py Outdated Show resolved Hide resolved
@MoritzWeber0 MoritzWeber0 force-pushed the pydantic-config-template branch 3 times, most recently from b62e344 to ca054ac Compare March 22, 2024 10:17
@MoritzWeber0 MoritzWeber0 force-pushed the pydantic-config-template branch from ca054ac to 66728d2 Compare March 22, 2024 10:21
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
2 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@MoritzWeber0 MoritzWeber0 merged commit ec23dae into main Mar 22, 2024
26 checks passed
@MoritzWeber0 MoritzWeber0 deleted the pydantic-config-template branch March 22, 2024 10:34
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.

Transform config_template.yaml into Pydantic object
2 participants