Add Mergeable to contract utils
#59
Merged
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
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #59 +/- ##
=========================================
+ Coverage 0 51.15% +51.15%
=========================================
Files 0 49 +49
Lines 0 2856 +2856
=========================================
+ Hits 0 1461 +1461
- Misses 0 1395 +1395
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
At this moment in time, `Mergeable` is defined in core [here](https://github.com/dbt-labs/dbt-core/blob/1a5d6922dddf9ffe018d8860bb2f2141594ddbbe/core/dbt/contracts/util.py#L27). We're curren't in the process of moving data artifacts of nodes defined in dbt-core's `nodes.py` into dbt-artifacts. Some of these data artifacts depend on `Mergeable`. We don't want artifacts to depend on core, thus `Mergeable` has to be moved _somewhere_ upstream. Given `Mergeable`'s similarity to `Replaceable` it made the most sense to move next to `Replaceable` here in dbt-common.
QMalcolm
force-pushed
the
qmalcolm--add-mergable
branch
from
0252104 to
4f27dd4
Compare
peterallenwebb
approved these changes
Feb 1, 2024
1 task
QMalcolm
added a commit
to dbt-labs/dbt-core
that referenced
this pull request
Feb 2, 2024
We're currently in the process of moving the "data resource" portion on nodes to `dbt/artifacts`. Some of those artifacts depend on `Mergeable` which has been defined on core. In order to move the data resources to `dbt/artifacts`, we thus need to move `Mergeable` upstream of core. We moved `Mergeable` to [dbt-common](https://github.com/dbt-labs/dbt-common) in dbt-labs/dbt-common#59, and released this change in [dbt-common 0.1.3](https://pypi.org/project/dbt-common/0.1.3/). As such as, in order to unblock some of the `dbt/artifacts` migration work, we first need to update references to `Mergeable` in core to use the `dbt-common` definition. NOTE: We include changing over to `Replaceable` from `dbt-common` in this commit. This is because there wasn't a clean way to do it. If I moved the imports of `Replaceable` on in the files where we updated `Mergeable` then we would have left `Replaceable` in an inbetween state. If we had moved all instances of `Replaceable`, it'd be out of scope for the change. As such, it makes more sense to do that as a separate changeset.
QMalcolm
added a commit
to dbt-labs/dbt-core
that referenced
this pull request
Feb 2, 2024
We're currently in the process of moving the "data resource" portion on nodes to `dbt/artifacts`. Some of those artifacts depend on `Mergeable` which has been defined on core. In order to move the data resources to `dbt/artifacts`, we thus need to move `Mergeable` upstream of core. We moved `Mergeable` to [dbt-common](https://github.com/dbt-labs/dbt-common) in dbt-labs/dbt-common#59, and released this change in [dbt-common 0.1.3](https://pypi.org/project/dbt-common/0.1.3/). As such as, in order to unblock some of the `dbt/artifacts` migration work, we first need to update references to `Mergeable` in core to use the `dbt-common` definition. NOTE: We include changing over to `Replaceable` from `dbt-common` in this commit. This is because there wasn't a clean way to do it. If I moved the imports of `Replaceable` on in the files where we updated `Mergeable` then we would have left `Replaceable` in an inbetween state. If we had moved all instances of `Replaceable`, it'd be out of scope for the change. As such, it makes more sense to do that as a separate changeset.
QMalcolm
added a commit
to dbt-labs/dbt-core
that referenced
this pull request
Feb 2, 2024
We're currently in the process of moving the "data resource" portion on nodes to `dbt/artifacts`. Some of those artifacts depend on `Mergeable` which has been defined on core. In order to move the data resources to `dbt/artifacts`, we thus need to move `Mergeable` upstream of core. We moved `Mergeable` to [dbt-common](https://github.com/dbt-labs/dbt-common) in dbt-labs/dbt-common#59, and released this change in [dbt-common 0.1.3](https://pypi.org/project/dbt-common/0.1.3/). As such as, in order to unblock some of the `dbt/artifacts` migration work, we first need to update references to `Mergeable` in core to use the `dbt-common` definition. NOTE: We include changing over to `Replaceable` from `dbt-common` in this commit. This is because there wasn't a clean way to do it. If I moved the imports of `Replaceable` on in the files where we updated `Mergeable` then we would have left `Replaceable` in an inbetween state. If we had moved all instances of `Replaceable`, it'd be out of scope for the change. As such, it makes more sense to do that as a separate changeset.
QMalcolm
added a commit
to dbt-labs/dbt-core
that referenced
this pull request
Feb 2, 2024
* Begin using `Mergeable` supplied by `dbt-common` We're currently in the process of moving the "data resource" portion on nodes to `dbt/artifacts`. Some of those artifacts depend on `Mergeable` which has been defined on core. In order to move the data resources to `dbt/artifacts`, we thus need to move `Mergeable` upstream of core. We moved `Mergeable` to [dbt-common](https://github.com/dbt-labs/dbt-common) in dbt-labs/dbt-common#59, and released this change in [dbt-common 0.1.3](https://pypi.org/project/dbt-common/0.1.3/). As such as, in order to unblock some of the `dbt/artifacts` migration work, we first need to update references to `Mergeable` in core to use the `dbt-common` definition. NOTE: We include changing over to `Replaceable` from `dbt-common` in this commit. This is because there wasn't a clean way to do it. If I moved the imports of `Replaceable` on in the files where we updated `Mergeable` then we would have left `Replaceable` in an inbetween state. If we had moved all instances of `Replaceable`, it'd be out of scope for the change. As such, it makes more sense to do that as a separate changeset. * Remove definition of `Mergeable` from dbt/contracts/util Although we've removed the definition of `Mergeable` we've ensured the import paths are still available. We do this because this is under `contracts`, and the sudden disappearance from the import path might cause issues for community members using dbt-core as a library. Ideally we'd define a `Mergeable` class here that inherits the `dbt-common` definition and raise a deprecation warning on instantiation. However, we don't have an established strategy to do so.
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.
resolves #58
Description
Adding
Mergeableso it's available upstream of dbt-coreChecklist
changie newto create a changelog entry