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

support bitwarden jinja style #1706

Merged
merged 1 commit into from
Feb 3, 2025
Merged

Conversation

wintonzheng
Copy link
Contributor

@wintonzheng wintonzheng commented Feb 3, 2025

Important

Adds support for Bitwarden parameters in workflow context, enhancing secure handling of sensitive information.

  • Behavior:
    • Adds support for Bitwarden parameters (BitwardenLoginCredentialParameter, BitwardenCreditCardDataParameter, BitwardenSensitiveInformationParameter) in WorkflowRunContext.
    • Updates initialize_workflow_run_context() in context_manager.py and service.py to handle new Bitwarden parameter types.
    • Modifies _set_up_workflow_context() in observer_service.py to pass organization for context setup.
  • Functions:
    • Adds register_bitwarden_login_credential_parameter_value(), register_bitwarden_credit_card_data_parameter_value(), and register_bitwarden_sensitive_information_parameter_value() in WorkflowRunContext.
    • Updates register_parameter_value() to handle new Bitwarden parameter types.
  • Error Handling:
    • Adds error logging for Bitwarden credential retrieval failures in context_manager.py.
    • Updates exception handling in execute_workflow() in service.py to log and handle initialization errors.

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

<!-- ELLIPSIS_HIDDEN -->

> [!IMPORTANT]
> Adds support for Bitwarden parameters in workflow context, enhancing secure handling of sensitive information in `context_manager.py`, `service.py`, and `observer_service.py`.
>
>   - **Behavior**:
>     - Adds support for Bitwarden parameters (`BitwardenLoginCredentialParameter`, `BitwardenCreditCardDataParameter`, `BitwardenSensitiveInformationParameter`) in `context_manager.py` and `service.py`.
>     - Updates `_set_up_workflow_context()` in `observer_service.py` to include `organization` parameter.
>     - Refactors `initialize_workflow_run_context()` in `context_manager.py` to handle secret parameters.
>   - **Models**:
>     - Adds `AWSSecretParameter`, `BitwardenLoginCredentialParameter`, `BitwardenCreditCardDataParameter`, `BitwardenSensitiveInformationParameter` to parameter handling in `context_manager.py`.
>   - **Functions**:
>     - Refactors `register_parameter_value()` in `context_manager.py` to handle new Bitwarden parameter types.
>     - Updates `execute_workflow()` in `service.py` to initialize workflow context with secret parameters.
>   - **Misc**:
>     - Adds error handling for Bitwarden operations in `context_manager.py`.
>     - Updates logging in `service.py` and `context_manager.py` for better error tracking.
>
> <sup>This description was created by </sup>[<img alt="Ellipsis" src="https://img.shields.io/badge/Ellipsis-blue?color=175173">](https://www.ellipsis.dev?ref=Skyvern-AI%2Fskyvern-cloud&utm_source=github&utm_medium=referral)<sup> for 7270c264dc99fd20af3dcf2a92152a812d35155a. It will automatically update as commits are pushed.</sup>

<!-- ELLIPSIS_HIDDEN -->
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 683f518 in 58 seconds

More details
  • Looked at 739 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 7 drafted comments based on config settings.
1. skyvern/forge/sdk/workflow/service.py:1555
  • Draft comment:
    Instead of using .strip(" {}") for jinja-style parameter processing, consider a more precise extraction (e.g. using regex) to avoid removing unintended characters.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. skyvern/forge/sdk/workflow/service.py:1555
  • Draft comment:
    Add a test case for jinja-style parameter keys to ensure proper context parameter initialization in for loop blocks.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. skyvern/forge/sdk/services/observer_service.py:338
  • Draft comment:
    Changed _set_up_workflow_context call now includes the organization. Verify that all callers supply a valid Organization object.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. skyvern/forge/sdk/workflow/context_manager.py:1555
  • Draft comment:
    Trimming loop_variable_reference with strip(' {}') supports jinja-style syntax; consider validating the trimmed key isn’t empty.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. skyvern/forge/sdk/workflow/service.py:1555
  • Draft comment:
    Update comment: 'backaward compatible with jinja style parameter' typo corrected and consider clarifying the purpose of trimming for future maintainers.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. skyvern/forge/sdk/workflow/service.py:244
  • Draft comment:
    Initialization of workflow run context now passes additional secret_parameters and organization; ensure this change is reflected in any external integrations.
  • Reason this comment was not posted:
    Comment did not seem useful.
7. skyvern/forge/sdk/workflow/context_manager.py:431
  • Draft comment:
    Consider handling cases where ContextParameter source value is not a dict more gracefully; this may affect downstream dependency resolution.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_YeSLG8ETVBBrLEXc


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

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! Reviewed everything up to 683f518 in 1 minute and 29 seconds

More details
  • Looked at 739 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. skyvern/forge/sdk/workflow/service.py:1555
  • Draft comment:
    Typo: 'backaward' should be 'backward'.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. skyvern/forge/sdk/workflow/service.py:1559
  • Draft comment:
    Using .strip(" {}") to trim jinja-style expressions can be brittle. Consider using a regex to robustly extract the variable inside '{{ }}'.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. skyvern/forge/sdk/workflow/service.py:1555
  • Draft comment:
    Typo in comment: 'backaward' should be 'backward', 'context parater' should be 'context parameter', and 'worklofw' should be 'workflow'.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. skyvern/forge/sdk/workflow/service.py:1559
  • Draft comment:
    Consider using a more robust extraction (e.g. regex) instead of .strip(' {}') for Jinja style variables, and add a check to raise an error if no matching parameter is found.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_AV5BKgbyAtxWNWTz


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

@LawyZheng LawyZheng merged commit 1ba2250 into main Feb 3, 2025
7 checks passed
@LawyZheng LawyZheng deleted the lawy/support-bitwarden-jinja-style branch February 3, 2025 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants