-
Notifications
You must be signed in to change notification settings - Fork 87
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
Implement migration sequencing (phase 2) #3009
base: main
Are you sure you want to change the base?
Conversation
def register_workflow_job(self, job: jobs.Job) -> MigrationNode: | ||
job_node = self._nodes.get(("JOB", str(job.job_id)), None) | ||
job_node = self._nodes.get(("WORKFLOW", str(job.job_id)), None) |
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.
align with ObjectInfo type enum
❌ 73/77 passed, 4 failed, 4 skipped, 1h49m41s total ❌ test_migration_sequencing_job_with_task_referencing_non_existing_cluster: TypeError: MigrationSequencer.__init__() missing 1 required positional argument: 'administrator_locator' (224ms)
❌ test_migration_sequencing_job_with_task_referencing_cluster: TypeError: MigrationSequencer.__init__() missing 1 required positional argument: 'administrator_locator' (2.028s)
❌ test_running_real_assessment_job_ext_hms: databricks.sdk.errors.platform.InternalError: Failed to list databases due to Py4JSecurityException. Update or reinstall UCX to resolve this issue. (22m1.346s)
❌ test_migration_sequencing_simple_job: TypeError: MigrationSequencer.__init__() missing 1 required positional argument: 'administrator_locator' (2.627s)
Running from acceptance #7539 |
ws_path = WorkspacePath(self._ws, object_id) | ||
object_owner = WorkspacePathOwnership(self._admin_locator, self._ws).owner_of(ws_path) | ||
else: | ||
raise ValueError(f"{object_type} not supported yet!") |
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.
where this exception is caught? it'll crash the assessment workflow if unhandled.
@@ -78,8 +78,8 @@ def as_message(self) -> str: | |||
|
|||
|
|||
class WorkflowTask(Dependency): | |||
def __init__(self, ws: WorkspaceClient, task: jobs.Task, job: jobs.Job): | |||
loader = WrappingLoader(WorkflowTaskContainer(ws, task, job)) | |||
def __init__(self, ws: WorkspaceClient, task: jobs.Task, job: jobs.Job, cache: WorkspaceCache | None = None): |
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.
don't add | None
as constructor dependencies - it leads to non-deterministic logic and subtle bugs that are harder to diagnose later.
f39bf38
to
ba8b4c6
Compare
3c34640
to
eb79746
Compare
Changes
Implement migration steps for notebooks and python files
Linked issues
Progresses #1415
Functionality
None
Tests