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

updated dbt core, snowflake, and bigquery versions #129

Merged
merged 10 commits into from
Jul 11, 2023
Merged

Conversation

britt-allen
Copy link
Contributor

@britt-allen britt-allen commented May 31, 2023

Fixes #101

@britt-allen britt-allen requested a review from ian-r-rose May 31, 2023 20:12
@britt-allen
Copy link
Contributor Author

@ian-r-rose
Copy link
Member

If you add your branch name to the list here you can temporarily build the docs in CI to see if there are any problems:

I had run into a problem where the built docs were landing in a different place so the logic around copying them into our github pages deploy broke. Totally fixable, but annoying. But it's possible that was an unintentional regression that has since been fixed.

@ian-r-rose
Copy link
Member

@britt-allen are you able to check/fix the docs build?

@britt-allen
Copy link
Contributor Author

Docs build complete and ready for merging @ian-r-rose

@ian-r-rose
Copy link
Member

Looks like the docs build had some problems, cf: https://cagov.github.io/dbt_docs_snowflake/index.html

@ian-r-rose
Copy link
Member

This is a breaking change to where dbt places the built project in 1.5 notice where it places the docs directory when running 1.4:

https://github.com/cagov/data-infrastructure/actions/runs/5136928618/jobs/9244319759#step:9:66

vs where it places the docs directory when running 1.5:

https://github.com/cagov/data-infrastructure/actions/runs/5149087990/jobs/9271648644#step:9:64

That is what I was referring to in #101. I think it would fix things to just change the cp statement.

@ian-r-rose
Copy link
Member

Found an issue for it: dbt-labs/dbt-core#7465

Looks like it was an unintentional change and will be fixed in the next release of dbt (1.5.2). So it might fix itself if we just wait and then bump the version again.

@britt-allen
Copy link
Contributor Author

So the current docs checks are not capable of catching this issue, correct? And that's why the checks passed despite the build actually being broken? @ian-r-rose

@britt-allen
Copy link
Contributor Author

britt-allen commented Jun 2, 2023

Should we write a test that doesn't let the check pass if the url is erroring out? I am also having a bit of trouble finding the cp statement you mentioned. May Slack you on Monday after a bit more digging. @ian-r-rose

@ian-r-rose
Copy link
Member

So the current docs checks are not capable of catching this issue, correct? And that's why the checks passed despite the build actually being broken? @ian-r-rose

Correct, there aren't really any docs checks right now, it's a very manual process

@ian-r-rose
Copy link
Member

The relevant lines are here:

# Generate snowflake dbt docs
dbt deps --project-dir=transform
dbt docs generate --project-dir=transform
cp -r transform/target docs/dbt_docs_snowflake
# Generate bigquery dbt docs
dbt deps --project-dir=transform-bigquery
dbt docs generate --project-dir=transform-bigquery
cp -r transform-bigquery/target docs/dbt_docs_bigquery

But it does look like if we wait a week or two this will be fixed with the next dbt release.

@ian-r-rose
Copy link
Member

Sigh, two steps forward, one step back. This is now blocked by dbt-labs/dbt-core#7937

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey Ian, can I get a tl;dr on why u removed some of the packages you did?

Copy link
Member

Choose a reason for hiding this comment

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

These are managed by poetry, so I'm not sure. It's a consequence of updating the versions above: somewhere in their dependency tree something required those packages before, and no longer does.

Copy link
Contributor Author

@britt-allen britt-allen left a comment

Choose a reason for hiding this comment

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

Can't approve my own PR, but looks good.

@britt-allen britt-allen requested a review from AeriShan-ODI July 11, 2023 17:13
@britt-allen
Copy link
Contributor Author

@AeriShan-ODI Ian and I both worked on this so would you mind reviewing and approving or soliciting more work if needed. it's my PR so I cannot approve. Thanks!

@ian-r-rose
Copy link
Member

ian-r-rose commented Jul 11, 2023

Just testing the docs build on this branch, then I'll revert and if things work well, should be good to go.

Edit: things seem to be working fine, this should be ready

@AeriShan-ODI
Copy link
Contributor

Tested, no issues found

@AeriShan-ODI AeriShan-ODI merged commit e16b98b into main Jul 11, 2023
@britt-allen britt-allen deleted the upgrade_dbt branch July 17, 2023 23:54
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.

Upgrade to dbt 1.5
3 participants