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

localhost:3000 everywhere #136

Closed
siddjellali opened this issue Jul 8, 2022 · 38 comments · May be fixed by #147
Closed

localhost:3000 everywhere #136

siddjellali opened this issue Jul 8, 2022 · 38 comments · May be fixed by #147
Labels
documentation Improvements or additions to documentation

Comments

@siddjellali
Copy link

siddjellali commented Jul 8, 2022

Hello team,

Could you please help me on that ?

image

Docker compose

  calendso:
    build:
      context: .
      dockerfile: Dockerfile
      args:
        - NEXT_PUBLIC_WEBAPP_URL=${NEXT_PUBLIC_WEBAPP_URL}
        - NEXT_PUBLIC_LICENSE_CONSENT=${NEXT_PUBLIC_LICENSE_CONSENT}
        - NEXT_PUBLIC_TELEMETRY_KEY=${NEXT_PUBLIC_TELEMETRY_KEY}
        - NEXTAUTH_SECRET=${NEXTAUTH_SECRET}
        - BASE_URL=${BASE_URL}
    container_name: calendso
    image: calendso/calendso:latest
    restart: always
    environment:
      NEXT_PUBLIC_LICENSE_CONSENT: $NEXT_PUBLIC_LICENSE_CONSENT
      DATABASE_URL: $DATABASE_URL
      NEXT_PUBLIC_WEBAPP_URL: $NEXT_PUBLIC_WEBAPP_URL
      GOOGLE_API_CREDENTIALS: $GOOGLE_API_CREDENTIALS
      NEXT_PUBLIC_TELEMETRY_KEY: $NEXT_PUBLIC_TELEMETRY_KEY
      EMAIL_FROM: $EMAIL_FROM
      EMAIL_SERVER_HOST: $EMAIL_SERVER_HOST
      EMAIL_SERVER_PORT: $EMAIL_SERVER_PORT
      EMAIL_SERVER_USER: $EMAIL_SERVER_USER
      EMAIL_SERVER_PASSWORD: $EMAIL_SERVER_PASSWORD 
      CALENDSO_ENCRYPTION_KEY: $CALENDSO_ENCRYPTION_KEY
      NODE_ENV: $NODE_ENV
      NEXT_PUBLIC_WEBSITE_URL: $NEXT_PUBLIC_WEBSITE_URL
      NEXT_PUBLIC_EMBED_LIB_URL: $NEXT_PUBLIC_EMBED_LIB_URL
      NEXTAUTH_URL: $NEXTAUTH_URL
      NEXTAUTH_SECRET: $NEXTAUTH_SECRET
      BASE_URL: $BASE_URL
      DOCKER_BUILDKIT: 0

I have all these variables configured in calcom docker

BASE_URL : https://agenda.xxxxxxx.com
NEXTAUTH_URL: https://agenda.xxxxxxx.com
NEXT_PUBLIC_APP_URL: https://agenda.xxxxxxx.com
NEXT_PUBLIC_WEBSITE_URL: https://agenda.xxxxxxx.com
NEXT_PUBLIC_WEBAPP_URL: https://agenda.xxxxxxx.com

What do i miss ?

image

+yellow: the license banner that won't go away (NEXT_PUBLIC_LICENSE_CONSENT: agree)

Thanks a lot for your support.

@nabilblk
Copy link

nabilblk commented Jul 8, 2022

+1 Same here

@PeerRich
Copy link
Member

PeerRich commented Jul 8, 2022

@krumware i believe their .env is not being picked up correctly?

@krumware
Copy link
Member

krumware commented Jul 8, 2022

Are you building your own image or pulling and running the image directly from dockerhub?

To clarify: At this time, for all URLs to reflect a custom URL, and because of the nature of the static site build, the environmental variables must be provided at build time for a custom container image which can then be run to reflect the custom URLs

@siddjellali
Copy link
Author

directly from dockerhub.

Do i need to create a fork and modify the variable below in the dockerfile ?
ARG NEXT_PUBLIC_WEBAPP_URL=http://localhost:3000

Why this variables cannot be modify as display in my docker compose above ?

@krumware
Copy link
Member

krumware commented Jul 8, 2022

You don't necessarily need to fork it, but you do need to tag and push your own image. This is a limitation of the nextjs build currently, as those variables are compiled into the frontend code at build time. The portions that are server side rendered or backend are able to use the variables, but not the files compiled into the static site.

@krumware
Copy link
Member

@siddjellali were you able to find a path forward?

@krumware krumware added the documentation Improvements or additions to documentation label Jul 18, 2022
@wzrdtales
Copy link

@krumware Why don't you import a normal config file in the main project and overwrite a placeholder in there with the environment variable during the container boot. Then you can dynamically configure this. Using non runtime (those vars are compile time) env vars is a bad practice actually for frontends.

@siddjellali
Copy link
Author

@siddjellali were you able to find a path forward?

Unfortunately not; I'm using portainer and submodule is not supported portainer/portainer#7240 (comment)

@krumware
Copy link
Member

@krumware Why don't you import a normal config file in the main project and overwrite a placeholder in there with the environment variable during the container boot. Then you can dynamically configure this. Using non runtime (those vars are compile time) env vars is a bad practice actually for frontends.

This has been brought to the teams attention in the past but is not my decision. There is some contention in the nextjs community around this as well, even though I personally share your opinion.

If you can point to an example to follow on a similar stack, it may help the core team with the info they need to make those architecture decisions.

@wzrdtales
Copy link

this is so easy you can do it in 2 minutes...

This is not an architecture question as well. This is plain simply: how do I configure my environment.

It is done simply like that:

A static json loader either in the bootstrap of the frontend framework, or even before the frontend runs. Then you load a json document from the static assets which contains the config and make the parameters available on the window object, or if doing it in the bootstrap in any proper equivalent.

Another common strategy is just using the same domain that the frontend is being served on and having the API available at /api/*.

@wzrdtales
Copy link

wzrdtales commented Jul 19, 2022

here a plain simple init routine that we use in endless variations:

sed -i "s%^URL:.*$% URL: '$APIURL'%" public/ApiConstants.json;

that is it.

even simpler if you just have certain keyword to replace.

@krumware
Copy link
Member

here a plain simple init routine that we use in endless variations:

sed -i "s%^URL:.*$% URL: '$APIURL'%" public/ApiConstants.json;

that is it.

even simpler if you just have certain keyword to replace.

I understand this method and have used it before as well, and I appreciate your passion for the issue.

Please open up an issue in https://www.github.com/calcom/cal.com with your recommendation and solution, and ideally a pull request related to the solution so that the core team can decide whether or not to implement this solution. There is existing discussion around runtime and static variable usage there as well.

If you feel this warrants a pull request solely in this repository, I'd be happy to review, test, and hopefully merge that as well.

@whallin
Copy link

whallin commented Jul 19, 2022

Running into the same problems as @siddjellali since we also deploy our Docker services via Portainer.

Deploying cal.com by the instructions in the README is definitely an option, no doubt there, but having an "official" Docker image ready to go would be way easier. Especially for users, like me, with Docker Swarm setups looking for an easier way of high availability with their containers.

Hopefully, we'll see some update shortly which addresses this. :)

@wzrdtales
Copy link

wzrdtales commented Jul 19, 2022

I understand this method and have used it before as well, and I appreciate your passion for the issue.

Not mean to be unfriendly, but I am not passionate about this issue, more concerned and annoyed about how amateur the current "delivery" is. I understand that it is not really your priority, since you really only care about your own service and not about the actual open source deployments out there, though. Which is fair enough, your business model seems quite clear and open core always deviate into that direction, but little more love doesn't hurt.

If I find some spare time I wouldn't have a problem providing you with an implementation of it, but don't wait might take some weeks from now since I am too involved in other duties.

@krumware
Copy link
Member

I'm a community member, not a compensated member of the team. For what it's worth, your situation is not unique, and many if not all of the other contributors to the docker side of the project have pressing duties outside of cal.com.

@krumware
Copy link
Member

krumware commented Jul 20, 2022

@whallin thanks for your info about your use with Portainer! Would you be willing to jump into the cal.com slack workspace to help with testing docker builds deployed using portainer?

@PeerRich
Copy link
Member

hey @wzrdtales as mentioned in this repository in multiple places
image
image

the /docker project is community ran. there are no economic incentives to do it badly. its just a very challenging task to correctly set up docker to work for everyone and no one in the core team (the engineers who work on the core product) have sufficient experience with any of these issues.

Please be mindful that open source is hard and if you know something better, feel free to open a PR with an explanation of the improvement

@whallin
Copy link

whallin commented Jul 20, 2022

@whallin thanks for your info about your use with Portainer! Would you be willing to jump into the cal.com slack workspace to help with testing docker builds deployed using portainer?

No worries. I'm on it!

@PeerRich
Copy link
Member

PeerRich commented Jul 21, 2022

@whallin thanks for your info about your use with Portainer! Would you be willing to jump into the cal.com slack workspace to help with testing docker builds deployed using portainer?

No worries. I'm on it!

that's the spirit!

@wzrdtales
Copy link

you can dislike honesty as much as you like of course, but when the "official" docker image is part of the organization of the same, no one expects it to be not official. And to be clear, that note is not clear enough, I didn't even see it until you mentioned it. If this is a community effort, I would either move it out of the organization, or at least rename the repository to unofficial, or unsupported to make it crystal clear.

@PeerRich
Copy link
Member

PeerRich commented Jul 21, 2022

you can dislike honesty as much as you like of course, but when the "official" docker image is part of the organization of the same, no one expects it to be not official. And to be clear, that note is not clear enough, I didn't even see it until you mentioned it. If this is a community effort, I would either move it out of the organization, or at least rename the repository to unofficial, or unsupported to make it crystal clear.

hate to break it to you but it's written all over the readme.

image

also on Docker Hub:

image

just because a repo is in an org, doesn't mean it's officially maintained by the company. There are many open source projects that have community-maintained repos next to commercially maintained repos (i.e. calcom/cal.com)

if docker was in https://github.com/calcom/cal.com then yes, we would officially support and maintain it. its not that we don't want to, we just don't have the experience for it, yet. (We plan to do this eventually)

rename the repository to unofficial

I believe it is mentioned enough in readme, description and docker hub.

or unsupported to make it crystal clear.

it is not unsupported. every now and then we make updates to Core which may break some Docker environments (and some don't). there are over 50k+ downloads of various docker images and maybe a handful of errors.

Just because a major version isn't working on the first day, doesn't mean its unsupported.

on your website I see you have expert experience with docker, why not help out?

image

we're also happy to compensate for contributions 🎉

@wzrdtales
Copy link

hate to break it to you but it's written all over the readme.

You didn't get my point, it's unclear in so far, yes you wrote it, but you have to see who reads it. It is not visually clear and lost in translation, just b/c you write something does not mean someone reads it. The more experienced you are the more likely you're to skip to the important part, which in this case is the docker image and its config params. Everything else we in our case (I wasn't the only one who completely didn't see this remark, in fact, the two other engineers who looked at it didn't note it as well) just ignored. But a docker name, and repository name that clearly says unsupported, or a repo outside of the org would have been clear immediately. You don't have to take anything from me serious o/c, that is totally on you, but this the perception of the receiver side.

we're also happy to compensate for contributions tada

I already wrote I will be happy to provide that, but also said it won't happen in the next weeks. If you have some patience and no one else came before us we're happy to help to get this a) dynamically configurable and b) ready for the modern container-driven world. Right now we're however at our capacity limit, so again, don't expect something during the next week.

@PeerRich
Copy link
Member

If you have some patience

of course, no worries! happy to engage when y'all have more capacity, unless @krumware and co. find a way earlier 🙏

Leopere pushed a commit to Leopere/calcom-docker that referenced this issue Aug 1, 2022
- added numerous environment variable changes such as implied defaults that can be overriden.
- skipped out on using git modules and just pull repo into build/launch step.  Adherance to license requires no repackaging and this solves this.
- cleaned up now unnecessary .env file.
- recycled environment section using yaml features.
- writing a few strings to config path to persist data between container starts that focus on cryptography and secrets.
- writing installed commit to the config path in case the end user needs to change the upstream git commit ID to a newer version for detection and automagic upgrades.
- added docker-compose.override.yml pattern to .gitignore to allow users to customize their local dev environment if they use docker-compose.yml
- wrote a dockerfile/container image which allows for uploading the base container to a private or public docker container registry without breaking the license rules.
- left .env ignore in case users wish to continue to use the old method.
- updated README.md to include updated simplified instructions.
- added start.sh script and wait-for-it.sh into the shell $PATH to allow for a potential future of allowing the main executable (node JS app) to run under a limited privilege user while still allowing the init scripts to be executed securely.
- added some input sanitation for certain critical variables.
- by default disabled/commented out the studio service as its not to typically be run to enforce better default deployment practices.  I would like to figure out what specific query to execute via the CLI instead of running a whole container to establish the first user in the end.
- wrote relatively unopinionated docker-compose.yml file to avoid causing problems for people trying to deploy this behind a reverse proxy for potential features such as TLS/HTTPS termination.
- upgraded compose version to latest '3.9' to be sure to enable most modern feature set.

Fixes calcom#87 by providing a working baseline with sober defaults.
Fixes calcom#88 by ensuring consistency across all containers Environment vars.
Fixes calcom#93 by allowing users to mount the application files within their IDE workspace, however, this will never solve for any times you will need to run yarn build steps.
Fixes calcom#99 by no longer using git submodules and just pulling a single commit depth copy of the ORIGIN repository on app bootstrap/first boot.
Fixes calcom#113 by no longer requiring build locally if the community maintainer of the Cal docker repository on GitHub will push this image to the hub.
Fixes calcom#121 by removing dependency on BuildKit this is done by simply deploying the app if its detected to be the first execution of this container be it due to no container persistence or a commit version upgrade from ORIGIN.
Fixes calcom#128 by removing dep on BuildKit
Fixes calcom#123 not replicatable and confirmed to be working in repository shipped state.
Fixes calcom#136 by building app on first launch from user define-able envvars which can be defined in numerous ways.
@kuzemkon
Copy link

kuzemkon commented Aug 2, 2022

IMHO this is a very weird issue. I've been involved in a lot of next.js projects and we were able easily successfully set up the dynamic frontend env variables configuration. Next.js has native support for it. The role of Next.Js is to provide a flexible website development environment for modern stateless deployment infrastructures, so it doesn't have limitations to do it.

The way we have it right now is a big antipattern for containerized applications. In the Docker world, the idea of an image is an artifact that could be executed in different environments being dynamically configurable.

I don't know why are you hesitant to implement dynamic frontend runtime variables as far as Next.js allows to do it really easy.

@PeerRich
Copy link
Member

PeerRich commented Aug 2, 2022

@kuzemkon i dont think there is any objection. i just think no one has gotten around of doing it yet. This would be great, would you be down to help us?

@kuzemkon
Copy link

kuzemkon commented Aug 2, 2022

@PeerRich I got it, thanks! Yes, I can try.

@PeerRich
Copy link
Member

PeerRich commented Aug 2, 2022

@PeerRich I got it, thanks! Yes, I can try.

that would be awesome!

@krumware
Copy link
Member

krumware commented Aug 2, 2022

@kuzemkon here is a feature request issue that provides some more context:
#141

also, here's a feature request for the database portion, which has some additional helpful context from @zomars: #142 (comment)

This requires changes in the core project, would be great if you can help!

@kuzemkon
Copy link

kuzemkon commented Aug 2, 2022

Thanks, @krumware. I'll try to dig into it this or next week.

@zomars
Copy link
Member

zomars commented Aug 2, 2022

I don't know why are you hesitant to implement dynamic frontend runtime variables as far as Next.js allows to do it really easy.

image

We do use automatic static optimization tho. Just because Next allows it doesn't mean it is a good use case for us. I'm curious about what other approach can be used here. The only one that comes to mind is using SSR everywhere which is not desired in the main application.

@wzrdtales
Copy link

wzrdtales commented Aug 2, 2022

Scheduling infrastructure for absolutely everyone.

If you actually mean this headline, then the app shouldn't be focused only on your own service.

Plus if you have SSR, there is no need for something like this auto static feature. Just generate static sites from the SSR output and either cache them or store them as files and serve.

It is usually this simple: If the app is only for your company, go static compile time. If the app is for more people than just yourself, config must be runtime.

@zomars
Copy link
Member

zomars commented Aug 3, 2022

If you actually mean this headline, then the app shouldn't be focused only on your own service.

That's not what I've said. "By not a good case for use us" I've meant for the application itself. Having everything SSRed would be a huge hit for performance which is not desired for the main application.

@satoshinotdead
Copy link

So, what's the top workaround until someone make a final solution?

We need to fork the whole app or edit files adding something to the start commands?

@krumware
Copy link
Member

Please test with latest images, as runtime variable substitution is now available.
Documentation updates are on the way.

@siddjellali
Copy link
Author

OOOOh 😍 Excellent !!
Can i close the ticket ?

@wzrdtales
Copy link

@siddjellali did you test it?

@siddjellali
Copy link
Author

siddjellali commented Nov 22, 2022

Yes of course ! and it's fine for me ^^
URL, picture, all seems good

@wzrdtales
Copy link

then its your ticket if its solved push the glory close button :p

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

Successfully merging a pull request may close this issue.

9 participants