-
Notifications
You must be signed in to change notification settings - Fork 80
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
Migrates Pipelines crawled during the assessment phase #2778
base: main
Are you sure you want to change the base?
Conversation
dfe9b45
to
96ccebf
Compare
738abc5
to
0858c5c
Compare
1ab4210
to
f28c72f
Compare
❌ 53/55 passed, 2 failed, 4 skipped, 7h23m52s total ❌ test_all_grants_for_other_objects: AssertionError: assert {'MODIFY', 'SELECT'} == set() (13m21.016s)
❌ test_all_grant_types: AssertionError: assert {('CATALOG', ...dummy_tmr5h')} == {('ANONYMOUS ..._firae'), ...} (13m34.716s)
Running from acceptance #7363 |
7dd9c06
to
71d98d0
Compare
71d98d0
to
e12dbd0
Compare
e12dbd0
to
28da733
Compare
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.
- make integration tests less flaky
- add cli command and document it in readme
- check if we can avoid creating a CSV mapping
|
||
def _migrate_pipelines(self): | ||
# get pipelines to migrate | ||
pipelines_to_migrate = self._pm.get_pipelines_to_migrate(self._pc) |
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.
why do we need a csv mapping for this one? we're copying pipelines, so there's less risk, right?
can you rather rename the old pipeline with [OLD]
suffix and stop it. then start the new one.
also i'm not seeing the updates of pipeline ids into jobs that trigger them via pipeline task.
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.
CSV mapping is to allow custom values for the optional parameters of clone API call: target_schema_name and target_pipeline_name. If doing a default 'as-is' migration, can directly migrate with default values. The csv will also allow for cleanup if the user does not want to migrate some pipelines.
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.
Maybe we can:
- Migrate all pipelines to one
catalog.schema
given input parameters from the cli. It is less flexible than a mapping, however, I do not know if it needs to be this flexible for pipelines. - Cleanup using a skip marker
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.
Added some pointers
self._pipeline_crawler = pipelines_crawler | ||
self._pipeline_mapping = pipeline_mapping | ||
|
||
def migrate_pipelines(self) -> 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.
This method looks redundant
8fee71e
to
1709ea2
Compare
Changes
Added PipelineMigrator to help with migration of DLT pipelines
Linked issues
Adds #3098
Functionality
Tests