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.

Sign up for GitHub

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

Jump to bottom

Make GoogleCloudServiceAccountDictProfileMapping dataset profile argument optional #837

Closed
oliverrmaa opened this issue Feb 6, 2024 · 3 comments · May be fixed by #839
Closed

Make GoogleCloudServiceAccountDictProfileMapping dataset profile argument optional #837

oliverrmaa opened this issue Feb 6, 2024 · 3 comments · May be fixed by #839
Labels
area:profile Related to ProfileConfig, like Athena, BigQuery, Clickhouse, Spark, Trino, etc dbt:run Primarily related to dbt run command or functionality profile:bigquery Related to BigQuery ProfileConfig

Comments

@oliverrmaa
Copy link

Context

Given the issue here and the discussion around it in slack, it follows that it would not be too far off for other profile mapping classes (like GCP) to have their schema (dataset in GCP terms) arguments made optional.

The configurations in dbt (profiles.yml, dbt_project.yml, model level configs etc.) are meant to provide the dataset. As such, me and my team's initial thoughts were to not provide any arguments here since usually we would rely on this being handled inside of dbt instead of it being hard coded at the DAG (orchestration) level.

Solution

I believe its possible modifying this line here and the associated code similarly to this issue here would be the right path.

Me and/or my colleague are happy to to attempt this modification if there are no objections to this proposed change. Please let us know, thanks
@dosubot DosuBot
Copy link

dosubot bot commented Feb 6, 2024 •
edited

🤖

That sounds like a great idea! Making the dataset profile argument optional could provide more flexibility for users. Your proposed solution seems like a step in the right direction. We encourage you and your colleague to go ahead and attempt the modification. Thank you for your willingness to contribute to the project!

This response is meant to be useful and save you time. It is not meant to be a precise solution, but rather a starting point for your own research.

Help me be more useful! Please leave a 👍 if this is helpful and 👎 if it is irrelevant.

If you want to continue the conversation, start your reply with @dosu-bot.

@dosubot dosubot bot added area:profile Related to ProfileConfig, like Athena, BigQuery, Clickhouse, Spark, Trino, etc dbt:run Primarily related to dbt run command or functionality profile:bigquery Related to BigQuery ProfileConfig labels Feb 6, 2024
@oliverrmaa oliverrmaa mentioned this issue Feb 7, 2024
Open
2 tasks
@oliverrmaa
Copy link
Author

@dosu-bot I have opened a PR #839 to address this issue and it has passed all tests and is ready for review, how do I request a review?

@dosubot DosuBot
Copy link

dosubot bot commented Feb 7, 2024

🤖

To request a review for your PR #839, you can follow these steps:

  1. Go to your pull request page on GitHub.
  2. On the right side of the page, you'll see a section titled "Reviewers".
  3. Click on the gear icon next to "Reviewers".
  4. From the dropdown, select the person or team you want to review your changes.

If you don't see the "Reviewers" section, it might be because you don't have the necessary permissions. In that case, you can leave a comment on the PR asking for a review from the relevant person or team.

Regarding the proposed change to make the dataset profile argument optional in GoogleCloudServiceAccountDictProfileMapping, it seems like a reasonable suggestion. The dataset is indeed usually handled inside of dbt and making it optional would provide more flexibility. However, please note that any changes to the codebase should be thoroughly tested to ensure they don't introduce any regressions or unexpected behavior.

As for the modification you mentioned, it would involve changing the required_fields list and the airflow_param_mapping dictionary in the GoogleCloudServiceAccountDictProfileMapping class. You would need to remove dataset from required_fields and handle the absence of dataset in airflow_param_mapping. This would likely involve checking if extra.dataset exists before trying to use it.

Please ensure that your changes are in line with the Cosmos Contributing Guide and that all tests pass before requesting a review. If you have any further questions or need more guidance, feel free to ask.

For more information, you can refer to the Cosmos codebase, specifically the GoogleCloudServiceAccountDictProfileMapping class, and the Cosmos Contributing Guide.

I hope this helps! If you have any other questions, don't hesitate to ask.

Sources

This response is meant to be useful and save you time. It is not meant to be a precise solution, but rather a starting point for your own research.

Help me be more useful! Please leave a 👍 if this is helpful and 👎 if it is irrelevant.

If you want to continue the conversation, start your reply with @dosu-bot.

@dosubot dosubot bot added the stale Issue has not had recent activity or appears to be solved. Stale issues will be automatically closed label May 8, 2024
@dosubot dosubot bot closed this as not planned Won't fix, can't repro, duplicate, stale May 15, 2024
@dosubot dosubot bot removed the stale Issue has not had recent activity or appears to be solved. Stale issues will be automatically closed label May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area:profile Related to ProfileConfig, like Athena, BigQuery, Clickhouse, Spark, Trino, etc dbt:run Primarily related to dbt run command or functionality profile:bigquery Related to BigQuery ProfileConfig
Projects
None yet
Milestone
No milestone
1 participant
@oliverrmaa