-
Notifications
You must be signed in to change notification settings - Fork 384
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
Significant Refactor #147
base: main
Are you sure you want to change the base?
Significant Refactor #147
Conversation
- 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.
If you can pull this fork to your local to test the only thing I'm unable to figure out is why after configuring everything successfully based off of the original why the end result is. yarn run v1.22.19
$ turbo run start --scope="@calcom/web"
• Packages in scope:
• Running start in 0 packages
Tasks: 0 successful, 0 total
Cached: 0 cached, 0 total
Time: 140ms
Done in 0.30s. |
I also did my best to avoid changing how it would possibly deploy historically but yet enabling future changes to be made which may break backward compatibility to make name changes and such. |
wow what a huge refactor! amazing. Whats missing to go from draft to ready for review? |
@PeerRich, for the most part, I'm just stuck on a few more minor details. I would recommend maybe validating my README.MD and committing any changes you think I should make or clarify. This was just a weekend for a fun project for me as I was passing through GitHub. All I can say is it bootstraps okay, it accepts and seems to validate the environment variable settings ok, but I don't know anything about NodeJS apps or Yarn to figure out how to debug what's not letting the web app itself launch. I think for the most part I managed to get 90+% of the original dockerfile and start stuff worked out in a relatively sober way but I'll see what you all think. |
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.
Overall is looking pretty good! Loving the new simplicity of the setup. I'll delegate my approval to @krumware since he's the docker guru in here.
@zomars, I mean, if you aren't the docker guru, that's almost better if it works for you, then I hit my target. |
@Leopere thanks for the contributions here. There's a lot to unpack here. It would be helpful if you could join the cal.com slack and we can chat about it |
@krumware I absolutely expected this to be declined haha I'll have a look for the Slack. |
To clean up visually environment variable definitions and defaults `ifndef` function has been added. It checks var for null, and if null, defines the default string based on defined defaults. Also, it prints variable strings to the console[stdout] for debugging. I also added more comments to start.sh's environment definitions for posterity.
@leog on Slack discussion it surged the idea of being about to create the first admin user via a custom seeder TS file as well so we don't expose unconfigured instances in the web. What do you think? |
@zomars that makes sense, we can remove the step in the wizard for now until app credentials gets implemented |
No need for removal. If the db has a user already the wizard won't show anyways |
Don't get me wrong; it's OK to allow the user to be created via the interface in case they don't deploy it via a standard container. I'm just suggesting it could be an excellent alternative to presenting user configuration to the internet. |
I see. So we would be covering all fronts. Great! |
Also, for added clarity, there are two modes for the actual boot invocation of the app itself. Invoked from this case switch here the
One of the biggest concerns with how this operates is simply that the git repository isn't automatically packaged in the Immutable Container itself; however, licensing of the Cal app itself dictates that repackaging for distribution is not allowed. Written consent would need to be provided to allow for this repackaging to be done via the community. |
Another detail that was brought up, and I will adjust the |
Finally, there may be blocking issues that were raised by @krumware about the immutability of the container and how the container starts. It would be difficult to exceed the boot speed of the previous configuration with how However, the concern about completely traditional container immutability may be a completely blocking issue if the Cal organization officials don't provide consent to allow repackaging the code base for pushing to the Docker Hub public Docker Registry. However, security-wise, there is a fully cryptographically secure chain of custody between the git repositories and any deployed container as we're using |
@@ -0,0 +1,103 @@ | |||
# Use postgres/example user/password credentials |
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.
It would be nice to modify the file, than deleting and creating a new one from a git history point of view. It will also be easy to compare what exactly changed.
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'm not sure the GitHub mobile view here is leaving out something I'm not sure what the suggestion is.
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.
What I meant is, that we already have a docker-compose.yaml file. So it would be nice to modify the same file instead of deleting it and creating a new file with the same functionality. This will help people understand better what exactly was changed in a particular revision.
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.
Seems great, just small concerns here and there.
Co-authored-by: Avinal Kumar <[email protected]>
Thank you for your review! Admittedly I made the mistake of not checking the find and replace on docker-compose in the documentation. I think a good presumption could be that users of |
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.
we're putting this on hold as we are doing some restructuring internally and will likely cherry-pick things from here, but not everything
thank you @Leopere for the contribution so far 🙏
Well I'd still really like to get this working or even help elsewhere. |
@Leopere, help us out here in the cheap seats: Can we use your work to create a mostly-working production environment? If so (and again, sorry for the very basic question) do we simply git clone your repository and head off to the races? Or does it combine with calcom's main branch in order to work? |
Significant Refactor.
Fixes #87 by providing a working baseline with sober defaults.
Fixes #88 by ensuring consistency across all containers Environment vars.
Fixes #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 #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 #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 #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 #128 by removing dep on BuildKit
Fixes #123 not replicatable and confirmed to be working in repository shipped state.
Fixes #136 by building app on first launch from user define-able envvars which can be defined in numerous ways.