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

[CHORE] Moving QueryLinter input objects to signature #3250

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

JCZuurmond
Copy link
Contributor

@JCZuurmond JCZuurmond commented Nov 12, 2024

Changes

Follow the more common pattern of expect objects in a class signature. Those objects are passed on from the context. Makes testing more straightforward

Breaking down the linked PR below.

Linked issues

Progresses #3045
Breaks up #3112

Tests

  • modified unit tests
  • modified integration tests

@JCZuurmond JCZuurmond added the internal this pull request won't appear in release notes label Nov 12, 2024
@JCZuurmond JCZuurmond self-assigned this Nov 12, 2024
@JCZuurmond JCZuurmond requested a review from a team as a code owner November 12, 2024 10:29
Copy link

github-actions bot commented Nov 12, 2024

❌ 89/90 passed, 1 flaky, 1 failed, 4 skipped, 1h15m44s total

❌ test_linter_from_context: TypeError: WorkflowLinter.refresh_report() missing 2 required positional arguments: 'sql_backend' and 'inventory_database' (6.912s)
TypeError: WorkflowLinter.refresh_report() missing 2 required positional arguments: 'sql_backend' and 'inventory_database'
[gw8] linux -- Python 3.10.15 /home/runner/work/ucx/ucx/.venv/bin/python
13:32 DEBUG [databricks.labs.ucx.install] Cannot find previous installation: Path (/Users/0a330eb5-dd51-4d97-b6e4-c474356b1d5d/.6ToO/config.yml) doesn't exist.
13:32 INFO [databricks.labs.ucx.install] Please answer a couple of questions to configure Unity Catalog migration
13:32 INFO [databricks.labs.ucx.installer.hms_lineage] HMS Lineage feature creates one system table named system.hms_to_uc_migration.table_access and helps in your migration process from HMS to UC by allowing you to programmatically query HMS lineage data.
13:32 INFO [databricks.labs.ucx.install] Fetching installations...
13:32 INFO [databricks.labs.ucx.installer.policy] Creating UCX cluster policy.
13:32 DEBUG [tests.integration.conftest] Waiting for clusters to start...
13:32 DEBUG [tests.integration.conftest] Waiting for clusters to start...
13:32 DEBUG [databricks.labs.ucx.install] Cannot find previous installation: Path (/Users/0a330eb5-dd51-4d97-b6e4-c474356b1d5d/.6ToO/config.yml) doesn't exist.
13:32 INFO [databricks.labs.ucx.install] Please answer a couple of questions to configure Unity Catalog migration
13:32 INFO [databricks.labs.ucx.installer.hms_lineage] HMS Lineage feature creates one system table named system.hms_to_uc_migration.table_access and helps in your migration process from HMS to UC by allowing you to programmatically query HMS lineage data.
13:32 INFO [databricks.labs.ucx.install] Fetching installations...
13:32 INFO [databricks.labs.ucx.installer.policy] Creating UCX cluster policy.
13:32 DEBUG [tests.integration.conftest] Waiting for clusters to start...
13:32 DEBUG [tests.integration.conftest] Waiting for clusters to start...
13:32 INFO [databricks.labs.ucx.install] Deleting UCX v0.49.1+3120241118133206 from https://DATABRICKS_HOST
13:32 INFO [databricks.labs.ucx.install] Deleting inventory database dummy_s63er
13:32 INFO [databricks.labs.ucx.install] Deleting cluster policy
13:32 INFO [databricks.labs.ucx.install] Deleting secret scope
13:32 INFO [databricks.labs.ucx.install] UnInstalling UCX complete
[gw8] linux -- Python 3.10.15 /home/runner/work/ucx/ucx/.venv/bin/python

Flaky tests:

  • 🤪 test_running_real_migration_progress_job (14m37.355s)

Running from acceptance #7423

@JCZuurmond JCZuurmond changed the title [CHOIR] Moving QueryLinter input objects to signature [CHORE] Moving QueryLinter input objects to signature Nov 12, 2024
Copy link
Collaborator

@nfx nfx left a comment

Choose a reason for hiding this comment

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

lgtm

TableMigrationIndex([]),
simple_ctx.directfs_access_crawler_for_queries,
simple_ctx.used_tables_crawler_for_queries,
None,
)
linter.refresh_report(sql_backend, simple_ctx.inventory_database)
linter.refresh_report()
Copy link
Collaborator

Choose a reason for hiding this comment

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

proper change here would be simple_ctx.query_linter.refresh_report()

@nfx nfx added the ready to merge this pull request is ready to merge label Nov 18, 2024
@JCZuurmond JCZuurmond force-pushed the fix/move-query-linter-objects-to-signature branch from 374f199 to ba8bcdc Compare November 18, 2024 12:16
@JCZuurmond JCZuurmond requested a review from a team as a code owner November 18, 2024 12:16
@nfx nfx enabled auto-merge November 18, 2024 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal this pull request won't appear in release notes ready to merge this pull request is ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants