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

Make credentials loading consistent between KedroContext._get_catalog() and catalog CLI #4165

Merged
merged 4 commits into from
Sep 20, 2024

Conversation

MigQ2
Copy link
Contributor

@MigQ2 MigQ2 commented Sep 15, 2024

Description

I found that credentials initialization is done differently in different cases:

Is there a good reason for this? If not, I think it's better to load the credentials in a consistent way and using the same methods everywhere

Why is this important?

I have a custom catalog class that extends KedroCatalog and overrides the _get_config_credentials() method because I get my credentials from a cloud secret management service instead of a local credentials.yml file.

This was making the command kedro catalog resolve fail because my credentials are not in the ConfigLoader

Checklist

  • Read the contributing guidelines
  • Signed off each commit with a Developer Certificate of Origin (DCO)
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added a description of this change in the RELEASE.md file
  • Added tests to cover my changes (I didn't add tests as I'm not adding new functionality and I believe this should be covered by existing tests)
  • Checked if this change will affect Kedro-Viz, and if so, communicated that with the Viz team

Update RELEASE.md

Signed-off-by: Miguel Rodriguez Gutierrez <[email protected]>
Signed-off-by: Miguel Rodriguez Gutierrez <[email protected]>
Signed-off-by: Miguel Rodriguez Gutierrez <[email protected]>
@ElenaKhaustova
Copy link
Contributor

Thank you, @MigQ2! This change makes sense to me. Could you please resolve the conflicts and I'll be able to approve the PR?

@MigQ2
Copy link
Contributor Author

MigQ2 commented Sep 19, 2024

Hi @ElenaKhaustova, conflicts solved!

Copy link
Contributor

@ElenaKhaustova ElenaKhaustova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you @MigQ2! ✨

Copy link
Member

@merelcht merelcht left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution @MigQ2 ! ⭐

@merelcht merelcht merged commit 13ff99e into kedro-org:main Sep 20, 2024
40 checks passed
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

Successfully merging this pull request may close these issues.

3 participants