-
Notifications
You must be signed in to change notification settings - Fork 35
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: Improve API key management #57
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a smart and sensible improvement. I appreciate that you're injecting it in for the CI process. One question/concern: what happens when I use the validate.sh
script while testing changes to the Dockerfile? It won't work anymore right? How do we solve for this? I've actually been making regular use of it as I test out new features. I guess we could re-add it during development, but that feels slightly janky to me.
What if the test key was commented out in the data-sources.xml file, and had a comment included above it that said something to the effect of "Uncomment this to use it during development but be sure to remove it before committing!" And then there was a CI check to make sure we didn't accidentally commit it?
(Also, apologies for taking so long to review this. the last few days have been a bit hectic for me) |
I made some adjustments based on our discussion.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great 👏 thanks for keeping at this!
In response to issue #10, I've modified our approach to API key management to ensure that test keys are only used during the CI test process. This change prevents test keys from being mistakenly used in production. I've configured the test API key to work with the
validate.sh
script, to test the service's endpoints during the CI process.