feat: add management of Helm applications #354
Conversation
|
@giovannibianco because we don't use Roles and ClusterRoles I removed them from the code. To have them still available I created this gist https://gist.github.com/konstan/ebcc78ece247a6febc3d1da8176b04c5 |
| _, services = connector.update(**kwargs) | ||
| if connector_name == HELM_CONNECTOR_KIND: | ||
| result, services, release = connector.update(deployment, **kwargs) | ||
| self.app_helm_release_params_update(release) | ||
| else: | ||
| result, services = connector.update(**kwargs) | ||
| if result: | ||
| self.job.set_status_message(result) |
There was a problem hiding this comment.
Having the same method returning different number of values is disturbing me.
The idea of having this connector interface is that (as much as possible) we don't have to write code specific to a connector in deployment_*.py files.
But here we have almost everywhere "if HELM_CONNECTOR".
Is there a way to improve that ?
For example we could update the interface of update() and start() to always return something like (result, services, extra: dict).
There was a problem hiding this comment.
Agreed. But to be precise, there are only two places where the Helm app deployment is treated special way
One is where you are pointing and another one is in getting application state.
I thought about changing the return value signature of the interface, but in the end resorted to the if as it was least intrusive.
If you are as well suggesting the three-tuple as return value for start() and update(), I will do that.
I'll see what can be done with get_application_state().
There was a problem hiding this comment.
TODO: refactor. Look for TODOs in the code.
| def store_yaml_file(data: dict, file_name, directory_path='.', | ||
| allow_unicode=True) -> str: | ||
| file_path = os.path.join(directory_path, file_name) | ||
| with open(file_path, 'w') as fd: | ||
| yaml.safe_dump(data, fd, allow_unicode=allow_unicode) | ||
| return file_path |
There was a problem hiding this comment.
Please see my other comments about writing to files that are not deleted or that are not ensured to be deleted.
In this case this method seems to be used only from one place (line 193) which give a temporary directory as directory_path but I think we need to ensure here that the directory is a temporary one.
(for example by creating the temporary directory here and returning its path instead of getting it as a function argument)
There was a problem hiding this comment.
This is handled by contract by the caller. Docu will be provided.
| """ | ||
| def __init__(self, **kwargs): | ||
| super().__init__(**kwargs) | ||
| self.helm = Helm(_NUVLAEDGE_SHARED_PATH, **kwargs) |
There was a problem hiding this comment.
Instead of _NUVLAEDGE_SHARED_PATH, job.nuvlaedge_shared_path should be used.
This class is used for both deployment on NE and on Cloud.
konstan
changed the title
Closes https://github.com/SixSq/tasklist/issues/2988
Closes https://github.com/SixSq/tasklist/issues/2990
Closes https://github.com/SixSq/tasklist/issues/3165