-
Notifications
You must be signed in to change notification settings - Fork 1
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
Deprecate use of process.env for runtime config #471
Conversation
- Replace process.env with window.env - Move env variables into a config.js file - Rework docker build to use file - Rework docker build to use static built files - Rework docker compose to load a config file - Add makefile utility matching station-data-portal - Add healthcheck to docker file
deployed at https://beehive.pacificclimate.org/pcex-fast/app/ for testing. |
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.
This looks great to me, @Nospamas . I can't say that I completely follow everything, but the performance is better, it cuts a lot of cruft and keeps our dependencies up to date. If you're happy with it, I am.
- name: Publish to Registry | ||
uses: docker/build-push-action@v1 | ||
with: | ||
username: ${{ secrets.pcicdevops_at_dockerhub_username }} | ||
password: ${{ secrets.pcicdevops_at_dockerhub_password }} | ||
dockerfile: docker/Dockerfile | ||
repository: pcic/climate-explorer-frontend |
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.
Are you concerned about the deprecation warning on this?
https://github.com/pacificclimate/climate-explorer-frontend/pull/471/checks#step:6:1
And maybe that would address this warning.
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.
Aye, I am concerned by it in so far as I do think we should fix it, but as it predates this issue so I haven't looked into fixing it just yet. I'll roll out these changes to each app (to fix startup times) then I'll go back and review updating that workflow to the latest version.
run: | | ||
git fetch --prune --unshallow | ||
echo "REACT_APP_CE_CURRENT_VERSION=$(git describe --tags --abbrev=0) ($(git rev-parse --abbrev-ref HEAD):$(git log -1 --format=%h))" >> $GITHUB_ENV | ||
echo "REACT_APP_APP_VERSION=$(git describe --tags --abbrev=0) ($(git rev-parse --abbrev-ref HEAD):$(git log -1 --format=%h))" >> $GITHUB_ENV | ||
- name: Build npm package |
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.
2 minutes for the build is so much better, especially given that it happens before deployment. Nice!
Matches configuration that was done as part of station-data-portal#186