search

DataJunction / dj

A metrics platform.
http://datajunction.io
MIT License
35 stars 15 forks source link

feat: pass env variables for reflection service and DJ core #1131

Closed sadath-12 closed 3 months ago

sadath-12 commented 3 months ago

Summary

Becomes easy when these components are separately deployed

Test Plan

Deployment Plan

netlify[bot] commented 3 months ago

Deploy Preview for thriving-cassata-78ae72 canceled.

Name Link
Latest commit efca9d9767bd9e3ee6bbb71902ea2727b9801f42
Latest deploy log https://app.netlify.com/sites/thriving-cassata-78ae72/deploys/66b0c1d95d589f00081f2e6b
samredai commented 3 months ago

@sadath-12 thanks for this PR. Can you expand on the PR description a bit? Is the purpose here so that you can have one centrally stored set of the same environment variables that you set for each of the different service types? You still intend to run these as separate services right? Also, should we add a DJ_QUERY_ prefix to the query service settings object?

cc: @agorajek and @shangyian, existing deployments would need to add these prefixes to our env vars once this change is deployed.

sadath-12 commented 3 months ago

@sadath-12 thanks for this PR. Can you expand on the PR description a bit? Is the purpose here so that you can have one centrally stored set of the same environment variables that you set for each of the different service types? You still intend to run these as separate services right? Also, should we add a DJ_QUERY_ prefix to the query service settings object?

cc: @agorajek and @shangyian, existing deployments would need to add these prefixes to our env vars once this change is deployed.

yes , we would deploy dj components separately which might not necessarily all be in same localhost . Also I was intending to add env variables as preference which would not change the default.

For query service it seemed fine since it doesn't call other services

shangyian commented 3 months ago

@sadath-12 I'm not sure why the prefix is needed? If these are all separate deployments, shouldn't each have their own .env file?

sadath-12 commented 3 months ago

@shangyian I dint notice the feature of pydantic to automatically pick up env variables first . so I think this pr might not be necessary

agorajek commented 3 months ago

+1 to what @shangyian said.