Skip to content

Fix dependency warnings#2703

Open
csafreen wants to merge 2 commits into
developfrom
csafreen/dependency_warning
Open

Fix dependency warnings#2703
csafreen wants to merge 2 commits into
developfrom
csafreen/dependency_warning

Conversation

@csafreen

@csafreen csafreen commented Jun 15, 2026

Copy link
Copy Markdown
Collaborator

The PR fixes 2 things

  1. A bug while creating result schema - if the job was triggered from the Jobs page in the UI then snapshot config is not None, it has attributes with Null and the result schema wouldn't be created.
  2. Fixes the dependency warning where Liquibase is storing sensitive information in a file - https://github.com/OHDSI/Data2Evidence/security/code-scanning/517 - Dependency/code scan actions: Data team #2650

Merge Checklist

Please cross check this list if additions / modifications needs to be done on top of your core changes and tick them off. Reviewer can as well glance through and help the developer if something is missed out.

  • Automated Tests (Jasmine integration tests, Unit tests, and/or Performance tests)
  • Updated Manual tests / Demo Config
  • Documentation (Application guide, Admin guide, Markdown, Readme and/or Wiki)
  • Verified that local development environment is working with latest changes (integrated with latest develop branch)
  • following best practices in code review doc

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR updates a couple of flow utilities to reduce warnings and improve runtime behavior, mainly by simplifying how Liquibase connection parameters are built and tightening snapshot-config detection in the cache creation flow.

Changes:

  • Refactors Liquibase CLI parameter construction (removes defaults-file generation, passes URL/password directly; improves error fallback message).
  • Suppresses chained CalledProcessError context when raising RuntimeError from Liquibase execution.
  • Tightens snapshot_copy_config detection so “empty” configs don’t block results-cache creation.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
plugins/flows/data_management/data_management_plugin/liquibase.py Refactors Liquibase invocation parameters and error handling/output parsing.
plugins/flows/base/create_cachedb_file_plugin/flow.py Improves snapshot config presence detection to avoid treating empty configs as configured.

Comment on lines +93 to 99
f"--url={connection_base_url}{connection_properties}",
f"--password={admin_password}",
]

if self.tenant_configs.authMode != AuthMode.JWT:
params.append(f"--username={admin_user}")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please solve the comment here

Comment on lines 91 to 95
f"--defaultSchemaName={self.schema_name}",
f"--liquibaseSchemaName={self.schema_name}",
f"--defaults-file={liquibase_properties}"
f"--url={connection_base_url}{connection_properties}",
f"--password={admin_password}",
]
Comment on lines +93 to 99
f"--url={connection_base_url}{connection_properties}",
f"--password={admin_password}",
]

if self.tenant_configs.authMode != AuthMode.JWT:
params.append(f"--username={admin_user}")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please solve the comment here


# Temporarily create liquibase.properties for sensitive values
# Won't be logged in traceback
with open(liquibase_properties, 'w') as file:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

the liquibase.properties is to prevent leaking credentials, why remove it?

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.

3 participants