Skip to content

chore(gckms): add file support for GCKMS_CONFIG env var #1956

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

adamazad
Copy link

Adds support for file paths in the GCKMS_CONFIG environment variable, allowing configurations to be loaded from a file or directly from an inline JSON string. It improves error handling with clear messages for missing or empty configurations and refines the .GckmsOverride.js logic for better maintainability. These changes enhance flexibility and simplify integration with various deployment setups.

@adamazad adamazad changed the title chore(gskms): add file support for GCKMS_CONFIG env var chore(gckms): add file support for GCKMS_CONFIG env var Dec 18, 2024
Copy link
Member

@james-a-morris james-a-morris left a comment

Choose a reason for hiding this comment

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

Hey! Thanks for the contribution 😎

I think it’s a good idea to refine the error logs. I wonder if we can refactor this change such that the file existence check (fs.existsSync) is only performed once. Currently, it’s being called in both branches of the if statement, which duplicates work.

@adamazad
Copy link
Author

adamazad commented Feb 15, 2025

Hi @james-a-morris, I made some changes, fs.existsSync is called once. It won't throw an error if you pass a JSON (when that's the case).

On a sidenote: I don't think it's a good idea to load put a module in dist/utils.

Copy link
Member

@james-a-morris james-a-morris left a comment

Choose a reason for hiding this comment

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

Hey! Thanks for these Looks good - however this current build fails 😞

@adamazad
Copy link
Author

@james-a-morris I updated the branch, and now CI is passing. :)

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