-
Notifications
You must be signed in to change notification settings - Fork 1
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 : add parameter of type connection (IASO,GCS..) #86
Conversation
6f829b7
to
34e617c
Compare
@pvanliefland Can you please check if you have time? |
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.
@cheikhgwane this is really good work - I still have a couple of suggestions to make it even better.
I think that we could also reexport the connection classes in sdk/__init__.py
, or at least in sdk/workspaces/__init__.py
openhexa/sdk/pipelines/parameter.py
Outdated
@@ -343,3 +522,19 @@ def get_all_parameters(self): | |||
return [self.parameter, *self.function.get_all_parameters()] | |||
|
|||
return [self.parameter] | |||
|
|||
|
|||
def is_connection_parameter(param: Parameter): |
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.
This is fine - but we could do something cleaner, ideally encapsulating the logic that builds a connection instance in Parameter.validate()
.
Do you think this would be possible?
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.
Yes, I've first tried that approach but for unknow (still searching) reason I couldn't load the connection config. But yes, I'll try to make it works.
openhexa/sdk/pipelines/parameter.py
Outdated
def accepts_multiple(self) -> bool: | ||
return False | ||
|
||
def validate(self, value: typing.Optional[typing.Any], *, allow_empty: bool = True) -> typing.Optional[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.
This feels duplicated between connection parameter types. We might need an intermediary class or helper methods to avoid copy pasting that kind of code.
openhexa/sdk/pipelines/pipeline.py
Outdated
@@ -97,7 +102,10 @@ def run(self, config: typing.Dict[str, typing.Any]): | |||
for parameter in self.parameters: | |||
value = config.pop(parameter.code, None) | |||
validated_value = parameter.validate(value) | |||
validated_config[parameter.code] = validated_value | |||
if is_connection_parameter(parameter): |
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.
See remark above, we might be able to put everything in validate
openhexa/sdk/pipelines/utils.py
Outdated
@@ -148,3 +158,23 @@ def get_local_workspace_config(path: Path): | |||
if key != "type": | |||
env_vars[stringcase.constcase(f"{slug}_{key.lower()}")] = str(value) | |||
return env_vars | |||
|
|||
|
|||
def get_connection_by_type(type: any, identifier: 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.
Might be unnecessary
tests/test_pipeline.py
Outdated
|
||
from openhexa.sdk.pipelines.parameter import Parameter, ParameterValueError | ||
from openhexa.sdk.pipelines.parameter import ( |
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.
This should be imported from openhexa.sdk.workspaces.connection
return super().validate(value, allow_empty) | ||
|
||
|
||
TYPES_BY_PYTHON_TYPE = { |
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.
Do you think that it would be a lot of added work to rename String
to StringType
, Boolean
to BooleanType
and so on?
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.
Let me try
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.
Let's go!
Enhance the parameter decorator by adding more supported types