unstract worker interface #272
Conversation
for more information, see https://pre-commit.ci
athul-rs
changed the title
athul-rs
changed the title
athul-rs
changed the title
|
There was a problem hiding this comment.
We don't need to add all the classes from docker class as an method here. Some of these methods won't make any sense in the context of k8s
| @abstractmethod | ||
| def _get_image(self) -> str: | ||
| pass | ||
|
|
||
| @abstractmethod | ||
| def _image_exists(self, image_name_with_tag: str) -> bool: | ||
| pass |
There was a problem hiding this comment.
I don't think these 2 needs to be part of interface. This is only required in docker_worker
| @abstractmethod | ||
| def normalize_container_name(self, name: str) -> str: | ||
| pass |
There was a problem hiding this comment.
This will be common for both k8s and docker I believe. So instead of making it an abstract class we could implement it here itself.
| def get_valid_log_message(self, log_line: str): | ||
| pass | ||
|
|
||
| @abstractmethod | ||
| def process_log_message(self, log_line: str, channel): | ||
| pass | ||
|
|
||
| @abstractmethod | ||
| def is_valid_log_type(self, log_type: str) -> bool: | ||
| pass | ||
|
|
||
| @abstractmethod | ||
| def get_log_type(self, log_dict: dict) -> str: | ||
| pass |
There was a problem hiding this comment.
All these 4 methods should be common to both k8s and docker. This is our own logics. So please add implementation here instead of making this an abstract method.
| @abstractmethod | ||
| def get_spec(self): | ||
| pass | ||
|
|
||
| @abstractmethod | ||
| def get_properties(self): | ||
| pass | ||
|
|
||
| @abstractmethod | ||
| def get_icon(self): | ||
| pass | ||
|
|
||
| @abstractmethod | ||
| def get_variables(self): | ||
| pass |
There was a problem hiding this comment.
@athul-rs I think this can also be common for both workers by making sure self.client.container.run is present in k8s implementation also. Or we can pass container object as an input to these methods. We might need to create an interface for this though. Or we could use the docker container datatype itself
|
@athul-rs I will be taking this up as well. Closing this PR |
What
Why
How
Database Migrations
Env Config
Relevant Docs
Related Issues or PRs
Dependencies Versions
Notes on Testing
Screenshots
Checklist
I have read and understood the Contribution Guidelines.