Skip to content
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

Feat/use envs for endpoint #40

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

johanravn
Copy link

@johanravn johanravn commented Oct 20, 2024

  • Add libexpat.so.1 installation to the dockerfile as it is a system dependency as it is required by rasterio.
  • Instead of hardcoding in backend endpoints in the front-end. Use environemnt variables setup suggest by Environment variables next.js. Makes it easier to switch endpoints on-the-fly.

mbsantiago and others added 3 commits October 20, 2024 14:29
Add libexpat.so.1 as a system dependency as it is required by rasterio
Instead of hardcoding in backend endpoints in the front-end use environemnt variables setup suggest by next.js. Makes it easier to switch endpoints on-the-fly in dev, test prod
Copy link

codecov bot commented Oct 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.47%. Comparing base (0868e7d) to head (571dcf6).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #40   +/-   ##
=======================================
  Coverage   66.47%   66.47%           
=======================================
  Files         179      179           
  Lines        8367     8367           
=======================================
  Hits         5562     5562           
  Misses       2805     2805           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mbsantiago
Copy link
Owner

Wow thanks @johanravn! This is a much cleaner way to handle the API endpoint. 👍

During the build process, could we ensure the API_ENDPOINT environment variable is left blank (or set to an empty string)? This way, the frontend will automatically point to the current host where the backend is running. Since the backend serves the built frontend, we don't need to explicitly specify the API's location. I ran into this issue when experimenting with hosting on a remote server, so I think it's worth addressing.

Also, it looks like the lint action failed due to a minor formatting issue. Could you please run npm run format on the frontend code and push the changes?

Thanks again!

@johanravn
Copy link
Author

Wow thanks @johanravn! This is a much cleaner way to handle the API endpoint. 👍

During the build process, could we ensure the API_ENDPOINT environment variable is left blank (or set to an empty string)? This way, the frontend will automatically point to the current host where the backend is running. Since the backend serves the built frontend, we don't need to explicitly specify the API's location. I ran into this issue when experimenting with hosting on a remote server, so I think it's worth addressing.

Also, it looks like the lint action failed due to a minor formatting issue. Could you please run npm run format on the frontend code and push the changes?

Thanks again!

Hi, thank you for a great project!

I changed the default to .env now, which can be overwritten by .env.development and .env.local (.local should not be commited into the repo). I am not sure what regarding the issue you outlined, have not digged too deep into the project yet. I only used the Dockerfile or the commands in the README.md to run the project.

pip install whombat

and

python -m whombat

Copy link
Owner

@mbsantiago mbsantiago left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @johanravn

Thanks again for the PR. I've reviewed the changes and they are looking good to me. I've added a few comments on changes that would be great to have, otherwise happy to merge this.

@@ -69,6 +69,11 @@ RUN --mount=type=cache,target=/root/.cache/uv \
# Then, use a final image without uv
FROM python:3.12-slim-bookworm

# Install system dependencies, including libexpat1 and clean up the cache
RUN apt-get update && apt-get install -y --no-install-recommends \
libexpat1 \
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we are at it can you please add the libsndfile1 library?

  • Also, can you please add this lines in the back/Dockerfile too?

This other Dockerfile is for dev development but also needs these dependencies.

@@ -1,7 +1,7 @@
import createAPI from "@/lib/api";

const api = createAPI({
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Can you please add something like the following:
const baseURL =
  process.env.NODE_ENV === "development"
    ? process.env.NEXT_PUBLIC_BACKEND_HOST || ""
    : "";

This way the environment variable is used in development (when the backend and frontend are separated) but not in production (when the backend serves the frontend in the same domain).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants