-
Notifications
You must be signed in to change notification settings - Fork 8
Implement fetch method, to populate template data on incarnation creation #517
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
base: main
Are you sure you want to change the base?
Conversation
| def __init__(self, hoster: Hoster) -> None: | ||
| self.hoster = hoster | ||
|
|
||
| async def get_template_variables(self, template_repository: str, template_version: str) -> dict[str, str]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add some automated tests around this?
| async with self.hoster.cloned_repository(template_repository, refspec=template_version) as repo: | ||
| template_config = TemplateConfig.from_path(repo.directory / "fengine.yaml") | ||
|
|
||
| return {k: v.get("default", "") for k, v in template_config.model_dump().get("variables", {}).items()} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why the casting into a dict? We can have better type safety in the code by just doing
| return {k: v.get("default", "") for k, v in template_config.model_dump().get("variables", {}).items()} | |
| return {k: v.get("default", "") for k, v in template_config.variables.items()} |
but then, also we have to be aware here that variables can be nested, complex objects. So this logic would have to be more recursive.
I would recommend adding functionality into the *VariableDefinition classes for getting an empty "example object" of that variable. This should then be easy to combine "up the tree" to ultimately get an "empty example" of a full template config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just added a commit to this branch which added a mock_data() method on the TemplateConfig object - including a test for it :-) check it out!
... to obtain an object with example values filled
No description provided.