[destination-bigquery] Removing excess permission requirements when dataset already exists

[kabeer27]

What

Solves:
#39330

Currently if a bigquery dataset in the destination already exists, we don't require datasets.create permission.

How

Refactoring the code a little bit to remove redundant checks for create permissions

Review guide

User Impact

Users can provide less permissions to the service account for their prod deployments

Can this PR be safely reverted and rolled back?

• YES 💚
• NO ❌

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
airbyte-docs	⬜️ Ignored (Inspect)	Visit Preview		Jun 26, 2024 4:04pm

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
16 out of 17 committers have signed the CLA.

✅ alafanechere
✅ davinchia
✅ kabeer27
✅ roman-yermilov-gl
✅ Adorism
✅ rodireich
✅ evantahler
✅ edgao
✅ ChristoGrab
✅ lazebnyi
✅ tolik0
✅ anjay-goel
✅ theyueli
✅ marcosmarxm
✅ gisripa
✅ jnr0790
❌ cmm-airbyte
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[Edward Gao (edgao)]

I think it would be better to just delete the checkHasCreateAndDeleteDatasetRole method completely. check is already doing a getOrCreateDataset, which means:

• if the dataset already exists, this validates our ability to see it
• if the dataset doesn't already exist, this validates our ability to create it

So checkHasCreateAndDeleteDatasetRole is entirely redundant with the rest of check.

[kabeer27]

I think it would be better to just delete the checkHasCreateAndDeleteDatasetRole method completely. check is already doing a getOrCreateDataset, which means:

• if the dataset already exists, this validates our ability to see it
• if the dataset doesn't already exist, this validates our ability to create it

So checkHasCreateAndDeleteDatasetRole is entirely redundant with the rest of check.

I'll remove that method entirely then, it did feel redundant but i assumed it might be throwing better error messages, but create permissions are also not mentioned in the docs so maybe just better to remove

[Edward Gao (edgao)]

🚛

[kabeer27]

I'm not sure about the merge process beyond this, anything required from my side? M. Marx (@marcosmarxm) Edward Gao (@edgao)

[M. Marx (marcosmarxm)]

kabeer27 one tests is failing, I'll check with Eduard and check what stpes are needed to fix them.

[Edward Gao (edgao)]

ah, you should just delete this line

airbyte/airbyte-integrations/connectors/destination-bigquery/src/test-integration/java/io/airbyte/integrations/destination/bigquery/BigQueryDestinationTest.java

Line 152 in 5b7873a

Arguments.of("nonBillableConfig", "Access Denied: BigQuery BigQuery: Streaming insert is not allowed in the free tier"));

the old test was doing an insertAll API call, which fails without a billing account.... but we never do that in reality (we either do a GCS copy, or create a bigquery WriteChannel). So this test case is entirely pointless.

[Edward Gao (edgao)]

conflicts were from converting the connector code to kotlin + some cleanup in the changelog, I just copied your changes over to the kotlin files (and merged the changelog/metadata stuff)