-
Notifications
You must be signed in to change notification settings - Fork 55
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
Add Dockerfile for serving containerised frontend #252
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.
I must be doing something wrong. Everything in the testing works for me except when I run docker compose up -d
then go to localhost:8080
it's not working. I'm getting minified react errors so I can't see what the exact cause is. I'm going to keep digging.
Edit: Got it to work. I removed everything and ran npm install
before docker compose again. Besides the two comments everything is good to me.
Edit 2: I've tested everything and it works for me. Note about Run the frontend in dev mode using: npm run start:dev - verify this works with proxied backend URLs
. If you want to test yourself you need to run the backend separately in the bff
dir with the command make run PORT=4000 MOCK_K8S_CLIENT=true
.
fe3f8cf
to
2cecf7e
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.
couple of nits and suggestions
* Dockerfile for serving the frontend and proxying calls to the bff. * An example docker-compose for running the stack. * Webpack config for proxying and url rewriting for the frontend in dev mode. * A frontend Makefile for building the docker image Signed-off-by: Alex Creasy <alex@creasy.dev>
I've addressed all the feedback given:
|
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.
Retested everything and it's working as expected
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.
/lgtm
@lucferbux: changing LGTM is restricted to collaborators In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ederign, Griffin-Sullivan, lucferbux The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/lgtm |
* chore: Add kind integration test, fixes kubeflow#251 * Remove unused IMG_VERSION env variable in pr build workflow
Description
This PR allows the frontend to be served in a container using nginx and adds:
How Has This Been Tested?
podman compose up -d
(swap podman for docker if that's your bag)localhost:8080
curl localhost:8080/api/v1/model-registry/
- this should return a valid response from the bffnpm run start:dev
- run the bff directly usingmake run PORT=4000 MOCK_K8S_CLIENT=true
in a separate terminal and verify the dashboard loads in a browser and the proxy functions by running:curl localhost:9000/api/v1/model-registry/
.make CONTAINER_TOOL=podman
(leave out the CONTAINER_TOOL param to use docker)Merge criteria: