Skip to content
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

workflow parameter validation #1028

Merged
merged 2 commits into from
Oct 23, 2024
Merged

Conversation

wintonzheng
Copy link
Contributor

@wintonzheng wintonzheng commented Oct 23, 2024

Important

Add workflow parameter validation to ensure values match expected types, raising InvalidWorkflowParameter on mismatch.

  • Validation:
    • Add InvalidWorkflowParameter exception in exceptions.py for invalid parameter types.
    • Update convert_value in WorkflowParameterType in parameter.py to validate parameter types and raise InvalidWorkflowParameter.
    • Integrate parameter validation in create_workflow_run_parameter in service.py.
  • Error Handling:
    • Raise WorkflowParameterNotFound in create_workflow_run_parameter if parameter ID is not found.
  • Refactoring:
    • Change value type to Any in convert_value and create_workflow_run_parameter for flexibility.

This description was created by Ellipsis for a3cbfde. It will automatically update as commits are pushed.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Changes requested. Reviewed everything up to 5ab53f3 in 54 seconds

More details
  • Looked at 123 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. skyvern/forge/sdk/workflow/models/parameter.py:141
  • Draft comment:
    Consider including workflow_permanent_id when raising InvalidWorkflowParameter for better traceability.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment suggests an improvement in error handling by including more context in the exception. This is a valid suggestion as it can help in debugging and tracing issues. The comment is actionable and clear, suggesting a specific change to improve code quality.
    The comment assumes that workflow_permanent_id is available and relevant in this context. Without seeing the rest of the codebase, it's unclear if this is the case. The suggestion might not be feasible if workflow_permanent_id is not accessible here.
    Even if workflow_permanent_id is not directly accessible, the suggestion highlights a potential area for improvement in error handling. It could prompt the author to consider how to include more context in exceptions, which is generally beneficial.
    The comment is a valid suggestion for improving error handling by including more context in exceptions. It should be kept as it provides a clear and actionable improvement to the code.

Workflow ID: wflow_nWkvuV8LzmogLIVf


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

skyvern/exceptions.py Show resolved Hide resolved
return value
lower_case = str(value).lower()
if lower_case in ["true", "false", "1", "0"]:
raise InvalidWorkflowParameter(expected_parameter_type=self, value=str(value))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The condition for raising InvalidWorkflowParameter is incorrect. It should raise the exception if the value is NOT in ['true', 'false', '1', '0'].

Suggested change
raise InvalidWorkflowParameter(expected_parameter_type=self, value=str(value))
if lower_case not in ["true", "false", "1", "0"]:

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Incremental review on a3cbfde in 45 seconds

More details
  • Looked at 83 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. skyvern/forge/sdk/workflow/service.py:9
  • Draft comment:
    InvalidWorkflowParameter is not imported. Add it to handle validation errors properly.
  • Reason this comment was not posted:
    Comment was on unchanged code.

Workflow ID: wflow_EixjYTMyQ1UlNpUu


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@wintonzheng wintonzheng merged commit 0e3da8d into main Oct 23, 2024
2 checks passed
@wintonzheng wintonzheng deleted the shu/fix_workflow_parameter_validation branch October 23, 2024 00:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant