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

fix: log full key def #5

Merged
merged 2 commits into from
Jul 2, 2024
Merged

fix: log full key def #5

merged 2 commits into from
Jul 2, 2024

Conversation

FluffyBucket
Copy link

Currently only the name from the spec would get logged if it was missing this makes it hard to easily find what exact value is missing for a given key.

Errors are given as: required key CLIENT_ID missing value, while the full key length is actually ECOM_CT_CLIENT_ID, where ECOM is the prefix and CT is the name of the config spec.

We do not need to use the alt name if it exists as the key field should always contain the key it uses to look up the env. Alt will only contain the name as specified in the envconfig struct tag.

Copy link

@papkos papkos left a comment

Choose a reason for hiding this comment

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

👍 Agree, the "missing" error message should specify which env you need to set for the process for it to work, not just the name in the nested struct.

Tests need to be fixed, and I wonder if it would make sense to add 2 new:

  1. One where there is no prefix, and no nesting
  2. One still using the prefix, but nesting the missing ennvar in a labeled struct.

@FluffyBucket FluffyBucket requested review from estensen and Zarux July 1, 2024 13:57
Copy link
Member

@hedlund hedlund left a comment

Choose a reason for hiding this comment

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

Very nice! 👍

Remember to squash history before/when merging.

@FluffyBucket FluffyBucket merged commit d3a94c5 into main Jul 2, 2024
5 checks passed
@FluffyBucket FluffyBucket deleted the fix/log-full-key-def branch July 2, 2024 14:19
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