Use env variable for backend services #15
Conversation
There was a problem hiding this comment.
This works as a quick fix for allowing configuration of the backend URL at the time of building the docker image, which is good.
However, it is leaving some stuff in a bit of a messy state (check my inline comments) so I'd prefer to have them properly cleaned up.
I would preferably fix the mentioned messyness in this PR, but for the sake of urgency I'd be OK with merging this PR, provided that things get cleaned up soon 🥵
| @@ -56,7 +56,9 @@ export class RequestApi extends Http { | |||
| } | |||
| return this.instance | |||
| .get<any>( | |||
| 'https://arpav.geobeyond.dev/api/v2/coverages/forecast-data?', | |||
| //'https://arpav.geobeyond.dev/api/v2/coverages/forecast-data?', | |||
There was a problem hiding this comment.
I would get rid of the commented out values throughout, as they are not needed anymore with this change
| @@ -3,7 +3,8 @@ export const API_URL = 'https://clima.arpa.veneto.it/backend/'; //process.env.RE | |||
| export const V2_API_URL = 'https://clima.arpa.veneto.it/backend/'; //process.env.REACT_APP_API_URL as string; | |||
There was a problem hiding this comment.
I would clean this file up, as it is getting confusing. Which of these variables are actually used in the code, if any?
| @@ -14,6 +14,7 @@ RUN yarn build | |||
| # Bundle static assets with nginx | |||
| FROM nginx:1.21.0-alpine as production | |||
| ENV NODE_ENV production | |||
| ENV REACT_APP_BACKEND_PUBLIC_URL https://clima.arpa.veneto.it | |||
There was a problem hiding this comment.
We now have three dockerfiles, which seem to differ only in how they are getting these env variables - how about consolidating into a single Dockerfile, which has default values for these env values suitable for dev, and which can then be parametrized when running docker build via passing --build-args for them?
There was a problem hiding this comment.
Coping with three different Dockerfiles is also introducing some unneeded complexity to the CI workflow, which I'd try to avoid as much as possible
There was a problem hiding this comment.
I totally agree that would be the optimal target but for now since this was the current status I don't want to change too much
|
@francbartoli I have taken the most relevant bits of this PR and used them in #17, with further changes that allow specifying the env when starting a docker container. As such, let's close this in favor of #17 |
This is breaking the current local run!!! PLEASE USE the following to run now:
export REACT_APP_BACKEND_PUBLIC_URL=https://arpav.geobeyond.dev yarn run start