-
Notifications
You must be signed in to change notification settings - Fork 3
refactor: migrate environment variables to runtime configuration #1121
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
base: main
Are you sure you want to change the base?
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
|
||
| DAO_GRAPH_URL=https://api.studio.thegraph.com/query | ||
| DAO_GRAPH_ID=rootstock-collective-governance-subgraph/version/latest | ||
| DAO_GRAPH_API_KEY=56839 # This key is for developing purposes only and have limited amount of queries |
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.
We should not remove this, this is for local development.
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.
But I'm only setting one env variable in AWS and that is THE_GRAPH_API_KEY. Having both is somewhat confusing, the application should be configured to expect one for each envs.
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.
@david-iov each graph implementation is different. TOK has their own implementation. DAO has their own implementation. If you want to unify both envs, which will require more effort, then we will have to setup some sort of plan to do this; and I don't think the scope of this PR is for this.
We can create a ticket to follow up on this; for now let's keep it there.
src/lib/the-graph.ts
Outdated
| // IMPORTANT: we don't expose the key to the client | ||
| export const fetchCrTheGraphEndpoint = `${process.env.THE_GRAPH_URL}/${process.env.THE_GRAPH_API_KEY}/${process.env.THE_GRAPH_ID}` | ||
| export const fetchDaoTheGraphEndpoint = `${process.env.DAO_GRAPH_URL}/${process.env.DAO_GRAPH_API_KEY}/${process.env.DAO_GRAPH_ID}` | ||
| export const fetchDaoTheGraphEndpoint = `${process.env.DAO_GRAPH_URL}/${process.env.THE_GRAPH_API_KEY}/${process.env.DAO_GRAPH_ID}` |
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.
The DAO will have its own key, unless that @Freshenext know something that I dont know.
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 only set THE_GRAPH_API_KEY in AWS.
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.
@david-iov please bring it up with both DAO and TOK as we may end up using different keys (it depends on the expected number of requests)
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 guess I'd leave both variable for now and also add the DAO_GRAPH_API_KEY in AWS, and we can discuss this in another scope.
The changes migrate from a build-time approach (where secrets were passed as Docker build arguments) to a runtime approach where environment variables will be loaded from AWS Parameter Store when the container starts. This is more secure and flexible, allowing environment variables to be updated without rebuilding the Docker image.