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

feat: support loading secrets when only present in dotenv #1879

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

haddowg
Copy link

@haddowg haddowg commented Nov 1, 2024

Please see the related PR on the secrets loader which adds support for defining secret env values in dotenv.

This change ensures we will always attempt to load secrets if the secret-loader is a dependency, as there may be no secret values (bref-ssm:...) in the lambda directly but there may be some in a dotenv file.

We keep the check for un-loaded secret values after the loader has run (if it was present) for cases where the dependency was missing or where the secret loading has somehow failed.

@haddowg haddowg marked this pull request as ready for review November 1, 2024 13:29
Copy link
Contributor

@GrahamCampbell GrahamCampbell left a comment

Choose a reason for hiding this comment

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

I don't think this makes sense, as we explicitly wanted to throw the exception to let people know, instead of this silently failing. If we did want to change that behaviour, you'd also need to delete lines 30-34.

@haddowg
Copy link
Author

haddowg commented Nov 1, 2024

I don't think this makes sense, as we explicitly wanted to throw the exception to let people know, instead of this silently failing. If we did want to change that behaviour, you'd also need to delete lines 30-34.

The suggested changes do not alter this behaviour, it will not silently fail, and will still throw an exception if the dependency is missing but you have a bref-ssm: prefixed env value.

If the dependency is present it will load the secrets the bref-ssm: prefixed values will no longer be present and it will return.

If the dependency is missing and you have no bref-ssm: prefixed values then it does nothing as previously.

@GrahamCampbell
Copy link
Contributor

Ah, I see.

@GrahamCampbell
Copy link
Contributor

Probably we should add a test to validate this.

@haddowg
Copy link
Author

haddowg commented Nov 1, 2024

Probably we should add a test to validate this.

Happy to, but not sure how practical that will be as the secret loader is a dev dependency currently.

Could remove it and change the ::class references to a local string constant and use call_user_func? Phpstan might not like that.
Let me know if that would be acceptable.

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.

2 participants