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

Add require_dbt_version to Hub API #146

Merged
merged 2 commits into from
Aug 19, 2022
Merged

Conversation

jtcohen6
Copy link
Contributor

@jtcohen6 jtcohen6 commented Aug 12, 2022

resolves dbt-labs/hub.getdbt.com#165

What is works_with used for? Is this the old name for packages? It's not being used for anything by recent versions of dbt-core.

I've added a totally new field to the hub API (require_dbt_version). I just started by reusing the existing field for expediency.

I tested the simple methods locally, against dbt_project.yml files that do and don't have require-dbt-version defined. Note that this will not work unless require-dbt-version is a valid semver string/list (as documented), so we probably want to add some validation logic to that effect.

As far as moving forward with this:

  • Not sure of the right way to end-to-end test this
  • Would we want to rerun the Hubcap script, to update existing package versions?

@@ -176,7 +177,7 @@ def make_spec(self, org, repo, package_name, packages, version):
"version": version,
"published_at": "1970-01-01T00:00:00.000000+00:00",
"packages": packages,
"works_with": [],
"works_with": require_dbt_version,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

require-dbt-version can be a string or a list (docs), so the resulting value in the Hub API can be a string or a list.

I added logic to handle this within dbt-core, rather than having that logic live here (= turning string into [string]).

@joellabes
Copy link
Contributor

I'd be keen to call it require_dbt_version - works with could be many things. dbt Core version and specific adapters spring to mind, but also more left field things like "does it work with Core and Cloud", or "does it work without superuser access to the database"...

@joellabes
Copy link
Contributor

  • Would we want to rerun the Hubcap script, to update existing package versions?

I think so, yeah!

@jtcohen6 jtcohen6 changed the title Parse require-dbt-version -> works_with Add require_dbt_version to Hub API Aug 15, 2022
@jtcohen6 jtcohen6 marked this pull request as ready for review August 17, 2022 09:49
Copy link
Contributor

@joellabes joellabes 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 good!

Note that this will not work unless require-dbt-version is a valid semver string/list (as documented), so we probably want to add some validation logic to that effect.

I don't see this anywhere; what are the consequences of an invalid string? Does dbt throw an exception if required version is something non semver, or does it ignore it?

I assume it ignores it and runs as normal? Which is ok to me, the only part that offend me on a principled level is that we might put inaccurate data into a long lived json file.

@jtcohen6
Copy link
Contributor Author

jtcohen6 commented Aug 17, 2022

Does dbt throw an exception if required version is something non semver, or does it ignore it?

Throwing an exception is the current behavior in dbt-labs/dbt-core#5651. So if it finds require_dbt_version: ["1.2.3.4"]:

dbt.exceptions.SemverException: "1.2.3.4" is not a valid semantic version.

But I could definitely switch it to just start ignoring require-dbt-version if it's invalid. Then it would be the same behavior as if the config were missing / empty.

FWIW, that's the same exception the package author would see any time they try running that package/project with an invalid require-dbt-version config. So I don't think it's very likely for an invalid one to slip in accidentally. I just bring it up because we're not running dbt or doing any dbt-related validation within the Hubcap process.

Once this PR is merged, I'm not sure of the right way to go about updating / re-deploying all existing package versions.

@joellabes
Copy link
Contributor

joellabes commented Aug 17, 2022

Exceptions at development time are even better! The reason I was at peace with it was mostly practical: we don't have a practical way to auto-open an issue against a misbehaving repo to warn them, and I didn't want the dbt team to keep getting pings about packages that we have no control over.

Once this PR is merged, I'm not sure of the right way to go about updating / re-deploying all existing package versions.

Is it not just this but everywhere? dbt-labs/hub.getdbt.com#1770 ?

@jtcohen6
Copy link
Contributor Author

jtcohen6 commented Aug 17, 2022

Is it not just this but everywhere? dbt-labs/hub.getdbt.com#1770 ?

You're right, it is just that but everywhere :)

I took some shortcuts / creative liberties to prepare that PR, by running modified components from Hubcap locally. Could do the same thing but more.

@dbeatty10
Copy link
Contributor

Is it not just this but everywhere? dbt-labs/hub.getdbt.com#1770 ?

Yep, I think you are exactly right about dbt-labs/hub.getdbt.com#1770 being the exemplar, @joellabes.

When a pull request like that is merged in the hub.getdbt.com repo, my understanding is that it immediately triggers a re-deploy on Netflify somehow. I haven't personally seen how Netflify is configured. But I've observed that it immediately redeploys when pull requests generated by the hubcap.py process are auto-merged.

@dbeatty10
Copy link
Contributor

Could do the same thing but more.

Yes, more (locally modified) cowbell should do the trick here. 🐮

Side note: I suspect the same general strategy will be needed if dbt-labs/hubcap #95 goes forward.

Copy link
Contributor

@dbeatty10 dbeatty10 left a comment

Choose a reason for hiding this comment

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

Thanks for diving into this across like three different repos @jtcohen6 !

To connect the dots for those interested, here are two other related pull requests:

  1. dbt-labs/dbt-core #5651
  2. dbt-labs/hub.getdbt.com #1770

This particular PR will generate JSON blobs like this (require_dbt_version being the new key+values):

    "id": "dbt-labs/dbt_utils/0.8.6",
    "name": "dbt_utils",
    "version": "0.8.6",
    "published_at": "1970-01-01T00:00:00.000000+00:00",
    "packages": [],
    "require_dbt_version": [
        ">=1.0.0",
        "<2.0.0"
    ],
    "works_with": [],
    "_source": {
        "type": "github",

@jtcohen6
Copy link
Contributor Author

I'm going to merge this one first. It won't be breaking for any existing versions of dbt-core, and it doesn't modify any existing Hub packages, but it will capture new ones that aren't included in the "manual backfill" I ran last night (dbt-labs/hub.getdbt.com#1779).

@jtcohen6 jtcohen6 merged commit 9311d86 into main Aug 19, 2022
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.

Feature: render require-dbt-version for each package
3 participants