search

rti / gbnc

Other
3 stars 1 forks source link

andrew's frontend #8

Closed rti closed 9 months ago

rti commented 9 months ago

Merges in the work of @andrewtavis from https://github.com/andrewtavis/gbnc

Closes #3

This approach is different from #4 as the frontend is still served via the unicorn fastapi server. There is no further containerization going on here.

I think #4 is the right approach on the long run. This PRs solution is very simple, keeping the deployment easier for now (one container instead of multiple). For real world production deployments, we most probably will still need #4.

exowanderer commented 9 months ago

The parts of the code that I can read make sense for why those edits were made. But I cannot comment further.

I would like to ask though: how do we generate frontend/dist to begin with?

I hoped that it would be built for us during the docker build. Now I assume that we have to enter the frontend directory, build the VueJS manually, and then build the docker.

Is that correct? If so

  1. How can we add this step into the Dockerfile without diving into docker-compose?
  2. Before we merge this, can we add the "build VueJS inside frontend director" language to the README.md?

Best, ew

exowanderer commented 9 months ago

Unrelated to the above: Do we agree to rename the director ./excellent-articles to ./json_input?

If so, could we please change that in this branch-PR as well?

rti commented 9 months ago

@exowanderer Thanks for the review. Very good catches.

I added the frontend build step to the docker container, so it is automatic now.

rti commented 9 months ago

Do we agree to rename the director ./excellent-articles to ./json_input?

I am fine with that. I merged the other branch from #9 to main, merged main into this one, so this is fixed too.

exowanderer commented 9 months ago

@exowanderer Thanks for the review. Very good catches.

I added the frontend build step to the docker container, so it is automatic now.

It is always helpful to have a noob onboard. I have no idea how to build VueJS, so I knew it was not in the Dockerfile.

PS: New T-shirt for Pregnant friends: "Noob On Board"

exowanderer commented 9 months ago

In the andrew-frontend branch, I am still unable to build the Docker because of the send_message in haystack.telemetry issue #6, but that is not related to my Change request above.