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

Decide whether to expose and how to handle DATA_CATALOG_ARGS in settings.py #1282

Closed
antonymilne opened this issue Feb 24, 2022 · 1 comment

Comments

@antonymilne
Copy link
Contributor

antonymilne commented Feb 24, 2022

In 0.18 we have the following settings which have moved over from registration hooks: CONFIG_LOADER_CLASS + CONFIG_LOADER_ARGS (used to be register_config_loader) and DATA_CATALOG_CLASS (from register_data_catalog). There is no DATA_CATALOG_ARGS. We discussed in #1064 (comment) whether DATA_CATALOG_ARGS should be exposed and decided not to.

Following #1280 I am wondering whether this decision will make migration from 0.17 difficult for some people, since some things you can do in 0.17 will no longer be possible. In particular, if I want to inject custom credentials from a source other than credentials.yaml (e.g. from AWS Secrets Manager) then this would no longer be possible.

Longer-term, as discussed in #1280 we probably need a better way of handling credentials. The task here is just to work out in the short-term (0.18) answers to the following:

  1. do we still let users inject custom credentials into DataCatalog.from_config as they can currently or if this is just no longer allowed?
  2. if it is allowed, how to handle DATA_CATALOG_ARGS to enable it given the below problems?

Problems with exposing DATA_CATALOG_ARGS:

  • the arguments outlined in [KED-2565] Register DataCatalog in settings instead of through the registration hook. #1064 (comment) still apply
  • CONFIG_LOADER_ARGS is used for arguments required for custom config loaders that are in addition to the arguments for AbstractConfigLoader. Entering credentials through DATA_CATALOG_ARGS is inconsistent with this model since it's already an argument in DataCatalog.from_config, and as @lorenabalan pointed out we don't have a well-defined AbstractDataCatalog interface
  • this means that if we did expose it currently then we'd need to make the user-specified credentials argument overwrite the existing credentials in the DataCatalog.from_config call. In addition to being inconsistent compared to behaviour of DATA_CATALOG_ARGS, this is destructive of any config in credentials.yaml
@antonymilne antonymilne changed the title Decide whether to expose DATA_CATALOG_ARGS in settings.py Decide whether to expose and how to handle DATA_CATALOG_ARGS in settings.py Feb 24, 2022
lvijnck pushed a commit to lvijnck/kedro that referenced this issue Feb 27, 2022
@merelcht
Copy link
Member

merelcht commented Mar 7, 2022

Discussed in grooming: do we still let users inject custom credentials into DataCatalog.from_config as they can currently or if this is just no longer allowed? The answer to this is that we'll look at this as part of the larger revamp of the configuration workflow. We'll specifically address how users should be using credentials.

@merelcht merelcht closed this as completed Mar 7, 2022
@merelcht merelcht moved this to Done in Kedro Framework Mar 7, 2022
lvijnck pushed a commit to lvijnck/kedro that referenced this issue Apr 7, 2022
Signed-off-by: Laurens Vijnck <laurens_vijnck@mckinsey.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants