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

ProfileMapping profile_args should raise a warning if they override mapped_params #696

Closed
jbandoro opened this issue Nov 20, 2023 · 2 comments · Fixed by #702
Closed

ProfileMapping profile_args should raise a warning if they override mapped_params #696

jbandoro opened this issue Nov 20, 2023 · 2 comments · Fixed by #702
Labels
area:config Related to configuration, like YAML files, environment variables, or executer configuration area:profile Related to ProfileConfig, like Athena, BigQuery, Clickhouse, Spark, Trino, etc priority:medium Medium priority issues are important issues that may have a workaround and medium impact profile:bigquery Related to BigQuery ProfileConfig

Comments

@jbandoro
Copy link
Collaborator

This issue was found in the Slack thread here where the user was using the GoogleCloudServiceAccountDictProfileMapping with the profile_args similar to:

profile_config = ProfileConfig(
        profile_name="default",
        target_name="dev",
        profile_mapping=GoogleCloudServiceAccountDictProfileMapping(
            conn_id="google_cloud_connection",
            profile_args={"method": "service_account"},
        ),
)

This would result in the dbt_profile_method class var that is mapped to dbt profile fields below:

def mapped_params(self) -> dict[str, Any]:
"Turns the self.airflow_param_mapping into a dictionary of dbt fields and their values."
mapped_params = {
"type": self.dbt_profile_type,
}
if self.dbt_profile_method:
mapped_params["method"] = self.dbt_profile_method
for dbt_field in self.airflow_param_mapping:
mapped_params[dbt_field] = self.get_dbt_value(dbt_field)
return mapped_params

to then be replaced when the profile is created below:

https://github.com/astronomer/astronomer-cosmos/blob/d28d3df7eacac74d0fbb016be70434bcc30c51be/cosmos/profiles/bigquery/service_account_keyfile_dict.py#L48C16-L52C10

because the merging of dictionaries with profile_args={"method": "service_account"} will replace the mapped_params that has {"method": ""service-account-json""}.

This issue is not limited to GoogleCloudServiceAccountDictProfileMapping and can happen with all of the profiles where there is overlap in the mapped params and the user provided profile args.

Cosmos should raise a warning if this happens to warn the user that they are replacing a cosmos mapped parameter.

Copy link

dosubot bot commented Nov 20, 2023

🤖

Hi @jbandoro,

Great to see you again! I hope you're doing well. I appreciate you bringing this issue to our attention. I'm diving into it right now and will follow up with a more comprehensive answer shortly. Thanks for your patience.

@dosubot dosubot bot added area:config Related to configuration, like YAML files, environment variables, or executer configuration area:profile Related to ProfileConfig, like Athena, BigQuery, Clickhouse, Spark, Trino, etc priority:medium Medium priority issues are important issues that may have a workaround and medium impact profile:bigquery Related to BigQuery ProfileConfig labels Nov 20, 2023
Copy link

dosubot bot commented Nov 20, 2023

🤖

It seems like you've got this under control, if you want help or have specific questions, let me know what I can do for you!


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.

pull bot pushed a commit to pgoslatara/astronomer-cosmos that referenced this issue Nov 23, 2023
…"method" (astronomer#702)

The issue is described in astronomer#696 which was discovered when a user was
creating a ProfileConfig with
`GoogleCloudServiceAccountDictProfileMapping(profile_args={"method":
"service-account"})` which was overriding the dbt profile method:

https://github.com/astronomer/astronomer-cosmos/blob/24aa38e528e299ef51ca6baf32f5a6185887d432/cosmos/profiles/bigquery/service_account_keyfile_dict.py#L21

when the profile args are mapped to the created profile below:


https://github.com/astronomer/astronomer-cosmos/blob/24aa38e528e299ef51ca6baf32f5a6185887d432/cosmos/profiles/bigquery/service_account_keyfile_dict.py#L42-L52

This is not an issue with the profile mapping example above and could
happen with any profile mapping by changing the "type" from
`dbt_profile_type` or "method" (if used) from `dbt_profile_method` in
the class.

The fix in this PR is to not allow args with "type" or "method" that are
different from the class variables in `profile_args`.

I think this is better than logging a warning because if either of those
fields are different the dbt run with the created profile will fail
anyways. This also allows backwards compatibility in the case users have
these already set in their profile args and it matches the class
variables.

Closes astronomer#696
arojasb3 pushed a commit to arojasb3/astronomer-cosmos that referenced this issue Jul 14, 2024
…"method" (astronomer#702)

The issue is described in astronomer#696 which was discovered when a user was
creating a ProfileConfig with
`GoogleCloudServiceAccountDictProfileMapping(profile_args={"method":
"service-account"})` which was overriding the dbt profile method:

https://github.com/astronomer/astronomer-cosmos/blob/24aa38e528e299ef51ca6baf32f5a6185887d432/cosmos/profiles/bigquery/service_account_keyfile_dict.py#L21

when the profile args are mapped to the created profile below:


https://github.com/astronomer/astronomer-cosmos/blob/24aa38e528e299ef51ca6baf32f5a6185887d432/cosmos/profiles/bigquery/service_account_keyfile_dict.py#L42-L52

This is not an issue with the profile mapping example above and could
happen with any profile mapping by changing the "type" from
`dbt_profile_type` or "method" (if used) from `dbt_profile_method` in
the class.

The fix in this PR is to not allow args with "type" or "method" that are
different from the class variables in `profile_args`.

I think this is better than logging a warning because if either of those
fields are different the dbt run with the created profile will fail
anyways. This also allows backwards compatibility in the case users have
these already set in their profile args and it matches the class
variables.

Closes astronomer#696
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:config Related to configuration, like YAML files, environment variables, or executer configuration area:profile Related to ProfileConfig, like Athena, BigQuery, Clickhouse, Spark, Trino, etc priority:medium Medium priority issues are important issues that may have a workaround and medium impact profile:bigquery Related to BigQuery ProfileConfig
Projects
None yet
1 participant