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

Feature/add quota project option #1345

Merged

Conversation

jcarpenter12
Copy link
Contributor

@jcarpenter12 jcarpenter12 commented Sep 11, 2024

resolves #1343
resolves #1344
resolves #1347

Docs PR dbt-labs/docs.getdbt.com#6054

Problem

Currently it is not possible to set a quota project in the profiles.yml configuration for BigQuery. This means that the quota project defaults to the quota project of the credentials. This can cause issues if using an account to auth that does not have the BQ API enabled (see bug here #1347 ). It also means that users can not run their models under a specific quota project if they want to.

Solution

Add an optional quota_project param in the profiles.yml that will allow a user to set a quota project if they want to. If they do not set it the behaviour will not change. The only override being applied is a config_option for the quota project that defaults to None anyway.

Checklist

  • I have read the contributing guide and understand what's expected of me
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • This PR has no interface changes (e.g. macros, cli, logs, json artifacts, config files, adapter interface, etc) or this PR has already received feedback and approval from Product or DX

@cla-bot cla-bot bot added the cla:yes label Sep 11, 2024
@colin-rogers-dbt colin-rogers-dbt added user docs [docs.getdbt.com] Needs better documentation community This PR is from a community member labels Sep 25, 2024
@jcarpenter12
Copy link
Contributor Author

@colin-rogers-dbt thank you for taking a look at this, do you know what the next steps with might look like? Am i right in thinking that if this change is merged it will be in the next release? Do you happen to know when that release is likely to be?
The reason I ask is that this change would resolve a bug in our CI pipelines (service accounts are defined in a project that does not have BQ API enabled, so they fail), so currently I am holding off making a workaround if this feature is in the next release

@colin-rogers-dbt
Copy link
Contributor

@jcarpenter12 Sorry for the radio silence here, this is a great PR! Ideally we would like to add a functional test (likely similar to how we test location config to make sure we don't break this in the future. If that's something you're comfortable doing, go for it, otherwise someone on our team can take this over and add it.

@jcarpenter12
Copy link
Contributor Author

@jcarpenter12 Sorry for the radio silence here, this is a great PR! Ideally we would like to add a functional test (likely similar to how we test location config to make sure we don't break this in the future. If that's something you're comfortable doing, go for it, otherwise someone on our team can take this over and add it.

apologies for not getting back to you sooner @colin-rogers-dbt , didn't get a notification for some reason. I will take a look and check my understanding and get back to you. Thanks for your help!

@jcarpenter12
Copy link
Contributor Author

@jcarpenter12 Sorry for the radio silence here, this is a great PR! Ideally we would like to add a functional test (likely similar to how we test location config to make sure we don't break this in the future. If that's something you're comfortable doing, go for it, otherwise someone on our team can take this over and add it.

apologies for not getting back to you sooner @colin-rogers-dbt , didn't get a notification for some reason. I will take a look and check my understanding and get back to you. Thanks for your help!

@colin-rogers-dbt i've had a crack at adding a functional test. I've tested it locally and it works with the temp GCP projects I setup to mirror what your CI jobs are doing. It has two use cases, one for if no quota project is set which is the default and another for whether an alternate quota project is set. I have tested locally and it passes for those two use cases but understand if this is not quite what you're after, happy to update it if so

Copy link
Contributor

@mikealfare mikealfare 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 a thorough PR @jcarpenter12! I appreciate testing both the case with and without the new config, the latter is often missed. And thank you for the docs PR as well!

@mikealfare mikealfare enabled auto-merge (squash) October 24, 2024 14:59
@jcarpenter12
Copy link
Contributor Author

jcarpenter12 commented Oct 25, 2024

Thanks for a thorough PR @jcarpenter12! I appreciate testing both the case with and without the new config, the latter is often missed. And thank you for the docs PR as well!

thanks @mikealfare I can see that the test is failing on the CI as the service account doesn't have the service usage consumer role on the BIGQUERY_TEST_ALT_DATABASE which I have used as the optional quota project. This wasn't an issue when running locally obviously as I had access to the project I setup. I am not sure how you would like to proceed on this one. Two options I would guess being; give the SA that role on that existing project or setup a new project specifically just for this quota test and then give that the role on the new project. Up to whoever owns these GCP projects but happy to support/test my side.

@mikealfare mikealfare merged commit 4e3f86e into dbt-labs:main Nov 1, 2024
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla:yes community This PR is from a community member ok to test user docs [docs.getdbt.com] Needs better documentation
Projects
None yet
4 participants