fix(sdk/dataflow): deprecate cluster and use env and platform_instance instead #8201
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
the
ingestion
hsheth2
requested changes
Jun 12, 2023
| @@ -35,11 +52,24 @@ class DataFlow: | |||
| url: Optional[str] = None | |||
| tags: Set[str] = field(default_factory=set) | |||
| owners: Set[str] = field(default_factory=set) | |||
| platform_instance: Optional[str] = None | |||
| env: Optional[str] = None | |||
There was a problem hiding this comment.
Previously we used cluster as the env value - why do we have both cluster and env now? I don't think we need this new instance variable
There was a problem hiding this comment.
Plus this is a breaking change and not deprecation. This needs to be documented in the PR doc. Plus we should not be changing the default as part of deprecation as that will cause existing deployments to have problems due to possible change of URNs.
There was a problem hiding this comment.
This isn't a breaking change yet, just a deprecation of cluster in favor of env + addition of platform_instance
However, we still need to make some changes:
- the deprecation should still be noted in the PR description + our updating datahub doc
- we should be printing a deprecation warning if folks use the old
clusterparam - we should throw an error if both env and cluster are provided
- all users of this class within datahub should be updated to use
env- I believe airflow is one, but there might be more
hsheth2
requested changes
Jun 13, 2023
metadata-ingestion/src/datahub/api/entities/datajob/dataflow.py
Outdated
Show resolved
Hide resolved
| @@ -35,11 +52,24 @@ class DataFlow: | |||
| url: Optional[str] = None | |||
| tags: Set[str] = field(default_factory=set) | |||
| owners: Set[str] = field(default_factory=set) | |||
| platform_instance: Optional[str] = None | |||
| env: Optional[str] = None | |||
There was a problem hiding this comment.
This isn't a breaking change yet, just a deprecation of cluster in favor of env + addition of platform_instance
However, we still need to make some changes:
- the deprecation should still be noted in the PR description + our updating datahub doc
- we should be printing a deprecation warning if folks use the old
clusterparam - we should throw an error if both env and cluster are provided
- all users of this class within datahub should be updated to use
env- I believe airflow is one, but there might be more
siddiquebagwan
suggested changes
Jun 14, 2023
metadata-ingestion/src/datahub/api/entities/datajob/dataflow.py
Outdated
Show resolved
Hide resolved
hsheth2
requested changes
Jun 14, 2023
metadata-ingestion/src/datahub/api/entities/datajob/dataflow.py
Outdated
Show resolved
Hide resolved
metadata-ingestion/src/datahub/api/entities/datajob/dataflow.py
Outdated
Show resolved
Hide resolved
metadata-ingestion/src/datahub/api/entities/datajob/dataflow.py
Outdated
Show resolved
Hide resolved
metadata-ingestion/src/datahub/api/entities/datajob/dataflow.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Harshal Sheth <hsheth2@gmail.com>
|
Deployment failed with the following error: |
Co-authored-by: Harshal Sheth <hsheth2@gmail.com>
Co-authored-by: Harshal Sheth <hsheth2@gmail.com>
Co-authored-by: Harshal Sheth <hsheth2@gmail.com>
Co-authored-by: Harshal Sheth <hsheth2@gmail.com>
hsheth2
approved these changes
Jun 15, 2023
hsheth2
changed the title
tusharm
pushed a commit
to tusharm/datahub
that referenced
this pull request
Jun 20, 2023
…e instead (datahub-project#8201) Co-authored-by: mohdsiddique <mohdsiddiquebagwan@gmail.com> Co-authored-by: Harshal Sheth <hsheth2@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Deprecation
In Dataflow, cluster argument is deprecated. Please use env instead.
Checklist