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

Update v12 to have limit and order_by #65

Merged
merged 1 commit into from
Oct 15, 2024

Conversation

QMalcolm
Copy link
Contributor

@QMalcolm QMalcolm commented Oct 12, 2024

Originally adding limit and order_by was done by #49, which added the v13 manifest. However, as these fields are additive and have default values, they aren't breaking changes. Thus, here we're adding them to the v12 manifest. This is in support of the currently open dbt-core PR dbt-labs/dbt-core#10532. Separately, v13 is being dropped by #66

Note this also drops the vars field from a few objects. It was originally added to support changes in core (dbt-labs/dbt-core#10793). However, those changes were reverted in core (dbt-labs/dbt-core#10813). Since vars as a field never went out in a GA release of dbt-core, it is safe for us to drop it.

@QMalcolm QMalcolm requested a review from a team October 12, 2024 03:49
Copy link
Member

@aranke aranke left a comment

Choose a reason for hiding this comment

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

Hmm, do we want to unpublish v13 in this PR?

Can we scope this PR to just update v12 to have limit and order_by and create another PR for deprecating v13?

@QMalcolm
Copy link
Contributor Author

Hmm, do we want to unpublish v13 in this PR?

Can we scope this PR to just update v12 to have limit and order_by?

We do want to unpublish v13

@QMalcolm
Copy link
Contributor Author

For others observing, we discussed over slack. I understood the comment as "are we sure we want to revert the creation of v13" when the question was really about "should this be 2 PRs". We're gonna split this into two PRs

Originally this work was done as part of #49.
However as they are additive fields that have a default values, they aren't
breaking changes. Thus here we're adding them to the v12 manifest. This is
in support of the currently open dbt-core PR dbt-labs/dbt-core#10532.

Note this also _drops_ the `vars` field from a few objects. It was originally added
to support changes in core (dbt-labs/dbt-core#10793). However,
those changes were reverted in core (dbt-labs/dbt-core#10813).
Since `vars` as a field never went out in a GA release of dbt-core, it is safe for us
to drop it.
@QMalcolm
Copy link
Contributor Author

QMalcolm commented Oct 15, 2024

Rebasing to drop commit bea8616 (the revert of v13), as that will now be handled by #66

@QMalcolm QMalcolm changed the title Remove v13 manifest and update v12 to have limit and order_by Update v12 to have limit and order_by Oct 15, 2024
@QMalcolm QMalcolm merged commit b5ff4af into main Oct 15, 2024
1 check passed
@QMalcolm QMalcolm deleted the qmalcolm--revert-v13-and-update-v12 branch October 15, 2024 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants