-
Notifications
You must be signed in to change notification settings - Fork 21
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
Build prod docker image #450
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.
This direction makes sense to me! We will probably want to make sure to check off some of the boxes for Express.js Production best practices before merging this in.
Marking this request changes because I would really really like to avoid putting a folder named app at the top level.
I pushed a commit in order to try building and running the docker container and seeing the effect of setting the environment variable flow through to the app running in the browser |
Ok so, I pushed up a few changes to address the issue around the Now, by default, when you bundle If you provide the environment variable You can test this out:
Now you can access the prod container at
@gabalafou @trallard what do you think of this approach? Does this stray too far from already defined patterns? |
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 good to me.
Just waiting to see the "app" folder renamed to "server."
Also I should mention that I'm really not qualified to review the workflow file, but it looks reasonable to me.
Co-authored-by: gabalafou <gabriel@fouasnon.com>
85f1151
to
01926a2
Compare
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 is in great shape.
I found one error that has to be fixed, it was my fault, I forgot to quote the values from process.env
😬
Co-authored-by: gabalafou <gabriel@fouasnon.com>
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.
Thanks @soapy1 !
Fixes #449 and #417
Description
This updates the release process to build a production conda-store-ui image. Changes include:
Pull request checklist
Additional information
It would be helpful to double check that the ui is working as expected. Especially anything having to do with the react router.
In order to test this, the following commands are useful:
Build the prod image
Set up services (this will also bring up the dev mode ui service on
localhost:8000
)Run the container
Now you can access the prod container at
localhost:8001