Make GoogleCloudServiceAccountDictProfileMapping dataset profile argument optional

[oliverrmaa]

Description

This PR follows the methodology in #683 by modifying the GCP GoogleCloudServiceAccountDictProfileMapping() profile_args not require dataset as a required argument.

Related Issue(s)

Closes #837

Breaking Change?

Not to my knowledge.

Checklist

• I have made corresponding changes to the documentation (if required)
• I have added tests that prove my fix is effective or that my feature works

[netlify]

✅ Deploy Preview for sunny-pastelito-5ecb04 canceled.

Name	Link
🔨 Latest commit	ef78fe9
🔍 Latest deploy log	https://app.netlify.com/sites/sunny-pastelito-5ecb04/deploys/6656ef226083a500096deac0

[oliverrmaa]

tagging @astro-cosmos-admins @tatiana for review, I wasn't able to tag anyone as reviewer since I don't have permissions so hopefully writing it as a comment is okay. Thank you

[jbandoro]

Thanks for your contribution @oliverrmaa!

I didn't realize "dataset"/"schema" was not required for the profiles file. I've changed it before by overriding the generate_schema_name as described in the docs where it by default concatenates the default target schema with the custom one which we found annoying.

If the default target schema from the profiles file is null do you still have to change the generate_schema_name macro?

[jbandoro]

I don't think this is needed since if dataset is already in profile_args it will be merged above in:

       {
            **self.mapped_params,
            "threads": 1,
            **self.profile_args,
        }

[jbandoro]

Do you know if having None in the profile is OK when this is formatted to yaml in BaseProfile.get_profile_contents this would result in profile like:

dataset: null
keyfile_json:
  private_key: '{{ env_var(''COSMOS_CONN_GOOGLE_CLOUD_PLATFORM_PRIVATE_KEY'') }}'
  private_key_id: '{{ env_var(''COSMOS_CONN_GOOGLE_CLOUD_PLATFORM_PRIVATE_KEY_ID'')
    }}'
  type: service_account
method: service-account-json
project: my_project
threads: 1
type: bigquery

Would the dataset: null line above cause any issue?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.40%. Comparing base (cb2a27a) to head (87d2e72).
Report is 6 commits behind head on main.

❗ Current head 87d2e72 differs from pull request most recent head ef78fe9

Please upload reports for the commit ef78fe9 to get more accurate results.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #839      +/-   ##
==========================================
- Coverage   95.72%   95.40%   -0.33%     
==========================================
  Files          60       57       -3     
  Lines        2926     2762     -164     
==========================================
- Hits         2801     2635     -166     
- Misses        125      127       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tatiana]

@oliverrmaa thank you very much for your contribution! Is there any chance you could address @jbandoro 's feedback? We may be able to add this as part of the 1.4.0 release later this week if you are.

[pankajastro]

Hey @oliverrmaa, Thanks for your contribution! Could you please rebase and respond to @jbandoro's feedback? we're excited to see this move forward.

[pankajastro]

Hi @oliverrmaa, I was wondering if you had a chance to read my previous comments. Are you still working on it, or would it be alright if I take it forward from here? Thank you!

[tatiana]

Since we consider this change very relevant, we ended up rebasing and addressing conflicts in a separate PR and adding @oliverrmaa as a co-author: #1017