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

Declare compatibility for previous artifact versions #5346

Merged
merged 4 commits into from
Jun 16, 2022

Conversation

jtcohen6
Copy link
Contributor

@jtcohen6 jtcohen6 commented Jun 8, 2022

resolves #5213

Description

We've been very aggressive about bumping artifact versions any time the artifact schema changes. For manifest.json, that's meant a bump for every minor version thus far, even when the schema changes are very minor (hah).

Sometimes, those changes do make it impossible for the latest version of dbt to deserialize an older manifest. Sometimes, they don't, and dbt can use the previous manifest version happily. These cases deserve different treatment, but today, we raise a version mismatch error in all cases.

This PR:

  • Develops the capability to differentiate between compatible and incompatible manifest versions, via a new base artifact class method is_compatible_version
  • Adds manifest v4 (produced by v1.0) to the set of "compatible" versions
  • Adds a functional test case

Very open to feedback on the implementation and nomenclature. If it's something we can feel good about, I'd really like to backport it to 1.1.latest for inclusion in an upcoming v1.1 patch (probably v1.1.2), since this will ease the migration path for v1.0 → v1.1 for anyone who uses state:modified / the "Slim CI" feature.

Checklist

@jtcohen6 jtcohen6 requested review from a team as code owners June 8, 2022 06:11
@cla-bot cla-bot bot added the cla:yes label Jun 8, 2022
Copy link
Contributor

@gshank gshank left a comment

Choose a reason for hiding this comment

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

This looks like a nice neat solution to me.

Copy link
Contributor

@ChenyuLInx ChenyuLInx left a comment

Choose a reason for hiding this comment

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

The changes look good to me!
One question that doesn't affect the merge of this PR: is it possible to automatically check compatibility by loading the manifest so that we don't need to always define it? But feels like that would have more drawbacks then benefits

@jtcohen6
Copy link
Contributor Author

is it possible to automatically check compatibility by loading the manifest so that we don't need to always define it? But feels like that would have more drawbacks then benefits

Yeah, I'd rather just keep it simple here, and test the end-user experience as closely as possible by including the actual contents of an actual manifest. It's not a guarantee that this will work in all cases, since this manifest is for a very simple project, but I hope that it would catch most errors that would arise during deserialization. It's also possible that the version upgrade has changed underlying dbt behavior, such as a core built-in macro, thereby triggering many many resources to rerun during "Slim CI."

I'm going to hold off on merging and backporting this PR until we've cut the final release of v1.1.1 (currently available as RC2).

@jtcohen6 jtcohen6 merged commit 6e8388c into main Jun 16, 2022
@jtcohen6 jtcohen6 deleted the jerco/previous-manifest-version-compat branch June 16, 2022 10:19
jtcohen6 added a commit that referenced this pull request Jun 16, 2022
* Add functional test

* Add is_compatible_version logic

* Add changelog entry

* Fix mypy
jtcohen6 added a commit that referenced this pull request Jun 16, 2022
* Add functional test

* Add is_compatible_version logic

* Add changelog entry

* Fix mypy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CT-599] [Bug] Upgrading from 1.0 to 1.1 breaks defer/state
3 participants