Initial implementation of pydantic validation for site adapters #231
Conversation
Codecov ReportBase: 98.85% // Head: 98.91% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #231 +/- ##
==========================================
+ Coverage 98.85% 98.91% +0.06%
==========================================
Files 55 55
Lines 2262 2387 +125
==========================================
+ Hits 2236 2361 +125
Misses 26 26
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
giffels
force-pushed
the
enable-pydantic-validation-site-adapter
branch
from
4cc1d9d to
856a6e7
Compare
giffels
force-pushed
the
enable-pydantic-validation-site-adapter
branch
from
5116617 to
1f6c452
Compare
giffels
force-pushed
the
enable-pydantic-validation-site-adapter
branch
from
1f6c452 to
772eac9
Compare
giffels
force-pushed
the
enable-pydantic-validation-site-adapter
branch
from
772eac9 to
ddd77b9
Compare
giffels
force-pushed
the
enable-pydantic-validation-site-adapter
branch
from
ddd77b9 to
09aea26
Compare
giffels
force-pushed
the
enable-pydantic-validation-site-adapter
branch
from
bfca82f to
2285f1f
Compare
giffels
force-pushed
the
enable-pydantic-validation-site-adapter
branch
from
2285f1f to
3a8ee41
Compare
giffels
force-pushed
the
enable-pydantic-validation-site-adapter
branch
from
3a8ee41 to
7dd3ba6
Compare
giffels
force-pushed
the
enable-pydantic-validation-site-adapter
branch
2 times, most recently
from
cf3cf65 to
6df1eb1
Compare
giffels
requested review from
a team and
mschnepf
and removed request for
a team
giffels
force-pushed
the
enable-pydantic-validation-site-adapter
branch
from
df1fbd4 to
d17dbb3
Compare
giffels
force-pushed
the
enable-pydantic-validation-site-adapter
branch
from
d17dbb3 to
eca3399
Compare
giffels
force-pushed
the
enable-pydantic-validation-site-adapter
branch
from
3b7b992 to
c9ab342
Compare
tardis/adapters/sites/htcondor.py
Outdated
| self._executor = getattr(self.configuration, "executor", ShellExecutor()) | ||
| bulk_size = getattr(self.configuration, "bulk_size", 100) | ||
| bulk_delay = getattr(self.configuration, "bulk_delay", 1.0) | ||
| self._configuration_validation_model = HTCondorAdapterConfigurationModel |
There was a problem hiding this comment.
As far as I understood things, you can (and actually intended to) set these as class attributes. This would make it clearer that the attribute really does not depend on instance data and it's not just a typo.
There was a problem hiding this comment.
Good point! Thanks for the hint.
|
|
||
| import logging | ||
|
|
||
| logger = logging.getLogger("cobald.runtime.tardis.adapters.sites.openstack") | ||
|
|
||
|
|
||
| class OpenStackAdapterConfigurationModel(SiteAdapterBaseModel): |
There was a problem hiding this comment.
I'm not sure whether this works in practice, but it looks like what you actually want is a Union where one part is only the application_credential and the other part is only the user+pwd+... part (both including auth_url).
So basically one class ApplicationConfigurationModel(SiteAdapterBaseModel): ..., one class UserConfigurationModel(SiteAdapterBaseModel): ... and then using Union[ApplicationConfigurationModel, UserConfigurationModel]. That could get rid of the manual either-or-exclusive test code. Probably a good idea to use a shorter naming scheme if you give this a try... 😅
There was a problem hiding this comment.
Good point. I will check pydantic documentation and give it try.
| """ | ||
| Property to access the configuration_validation model of a site adapter | ||
| implementation and ensuring that all sub-classes of the SiteAdapter have | ||
| a _configuration_validation_model class variable. |
There was a problem hiding this comment.
The description and usage seem to confuse class and instance attributes. From the description and usage it seems you wanted class attributes, the assignments indicate instance attributes and the error mixes both. Based on the semantics I would guess you actually want class attributes.
Can you adjust this to be consistent? I think it needs to be addressed just here and when assigning the models to the classes/instances.
giffels
force-pushed
the
enable-pydantic-validation-site-adapter
branch
from
10461fc to
65ad254
Compare
Co-authored-by: Max Fischer <[email protected]>
This pull request enables pydantic input validation for the configuration of the site adapters used in
TARDIS. In addition, it enables support for OpenStack application credentials.Implement the general concept on how to invoke pydantic input validation on site adapters.
AdapterConfigurationModelself._configuration_validation_modelis point toAdapterConfigurationModelself.configuration, the validation is invoked in theSiteAdapterinterface implementation of theconfigurationproperty.Add configuration model for
Add support for OpenStack application credentials