-
Notifications
You must be signed in to change notification settings - Fork 1
feat: add secret parameter type for pipeline parameters(HEXA-1457 ) #373
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3180,6 +3180,7 @@ enum ParameterType { | |
| int | ||
| postgresql | ||
| s3 | ||
| secret | ||
| str | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -389,11 +389,74 @@ def validate(self, value: typing.Any | None) -> File: | |
| raise ParameterValueError(str(e)) | ||
|
|
||
|
|
||
| class Secret: | ||
| """Marker type for secret/password pipeline parameters. | ||
|
|
||
| Use as the ``type`` argument of the ``@parameter`` decorator to indicate that the parameter value is sensitive | ||
| and should be hidden in the OpenHEXA web interface. The pipeline function will receive the value as a plain | ||
| ``str`` at runtime. | ||
|
|
||
| Example:: | ||
|
|
||
| @parameter("iaso_token", type=Secret, name="IASO token", required=True) | ||
| @pipeline("my-pipeline") | ||
| def my_pipeline(iaso_token: str): | ||
| ... | ||
| """ | ||
|
|
||
| pass | ||
|
|
||
|
|
||
| class SecretType(ParameterType): | ||
| """Type class for secret/password string parameters. Values are treated as plain strings at runtime.""" | ||
|
|
||
| @property | ||
| def spec_type(self) -> str: | ||
| """Return a type string for the specs that are sent to the backend.""" | ||
| return "secret" | ||
|
|
||
| @property | ||
| def expected_type(self) -> type: | ||
| """Returns the python type expected for values.""" | ||
| return str | ||
|
|
||
| @property | ||
| def accepts_choices(self) -> bool: | ||
| """Secrets don't support choices.""" | ||
| return False | ||
|
|
||
| @property | ||
| def accepts_multiple(self) -> bool: | ||
| """Secrets don't support multiple values.""" | ||
| return False | ||
|
|
||
| @staticmethod | ||
| def normalize(value: typing.Any) -> str | None: | ||
| """Strip whitespace and convert empty strings to None.""" | ||
| if isinstance(value, str): | ||
| normalized_value = value.strip() | ||
| else: | ||
| normalized_value = value | ||
|
|
||
| if normalized_value == "": | ||
| return None | ||
|
|
||
| return normalized_value | ||
|
|
||
| def validate_default(self, value: typing.Any | None): | ||
| """Validate the default value configured for this type.""" | ||
| if value == "": | ||
| raise ParameterValueError("Empty values are not accepted.") | ||
|
|
||
| super().validate_default(value) | ||
|
|
||
|
|
||
| TYPES_BY_PYTHON_TYPE = { | ||
| "str": StringType, | ||
| "bool": Boolean, | ||
| "int": Integer, | ||
| "float": Float, | ||
| "Secret": SecretType, | ||
| "DHIS2Connection": DHIS2ConnectionType, | ||
| "PostgreSQLConnection": PostgreSQLConnectionType, | ||
| "IASOConnection": IASOConnectionType, | ||
|
|
@@ -438,6 +501,7 @@ def __init__( | |
| | int | ||
| | bool | ||
| | float | ||
| | Secret | ||
| | DHIS2Connection | ||
| | IASOConnection | ||
| | PostgreSQLConnection | ||
|
|
@@ -460,7 +524,10 @@ def __init__( | |
| self.code = code | ||
|
|
||
| try: | ||
| self.type = TYPES_BY_PYTHON_TYPE[type.__name__]() | ||
| if isinstance(type, ParameterType): | ||
| self.type = type | ||
| else: | ||
| self.type = TYPES_BY_PYTHON_TYPE[type.__name__]() | ||
|
Comment on lines
+527
to
+530
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this change required ? Just curious
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. runtime.py passes a ParameterType instance directly to Parameter.init, so this check was needed to handle that path and it worked. let me know if it should be this way. not so sure if it is the perfect way.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems a smell that something is wrong, because conceptually we just want to add a new type to but we modify the generic logic for all types I think #374 would solve it (not particularly polished nor tested) |
||
| except (KeyError, AttributeError): | ||
| valid_parameter_types = [k for k in TYPES_BY_PYTHON_TYPE.keys()] | ||
| raise InvalidParameterError( | ||
|
|
@@ -621,6 +688,7 @@ def parameter( | |
| | int | ||
| | bool | ||
| | float | ||
| | Secret | ||
| | DHIS2Connection | ||
| | IASOConnection | ||
| | PostgreSQLConnection | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -308,7 +308,7 @@ def get_pipeline(pipeline_path: Path) -> Pipeline: | |
| # Convert args spec to parameter kwargs | ||
| param_kwargs = {k: v["value"] for k, v in parameter_args.items()} | ||
|
|
||
| parameter = Parameter(type=type_class.expected_type, **param_kwargs) | ||
| parameter = Parameter(type=type_class, **param_kwargs) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this change required ? Just curious
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Before I was having an error of having the secret reaching in the database treated as "str" I found that without this fix, SecretType().expected_type resolves to str, meaning Secret parameters would silently serialize as "type": "str" and never reach the frontend as masked.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I think the type logic smells wrong for the Secret and this seems to fix the symptom but not the underlying issue What do you think of this #374 ? (not particularly polished nor tested) |
||
| pipeline_parameters.append(parameter) | ||
|
|
||
| except KeyError as e: | ||
|
|
||
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.
To modify the files under
graphql/graphql_client/you need to follow https://github.com/BLSQ/openhexa-sdk-python/blob/0f795c065543956e8487d3b1a126e65a3a09ea1a/README.md#codegen-from-the-graphql-schemaThose files are auto generated based on
schema.generated.graphqlThere 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.
I know I'm off today :) but felt to address the comments. It slipped my mind that files under graphql/graphql_client/ are auto-generated. I've added secret to schema.generated.graphql and re-ran ariadne-codegen to regenerate enums.py instead of editing it manually.