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

Significant Refactor #147

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Significant Refactor #147

wants to merge 5 commits into from

Conversation

Leopere
Copy link
Contributor

@Leopere Leopere commented Aug 1, 2022

Significant Refactor.

  • 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 #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.

Colin added 2 commits August 1, 2022 13:39
- 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.
@Leopere
Copy link
Contributor Author

Leopere commented Aug 1, 2022

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.

@Leopere
Copy link
Contributor Author

Leopere commented Aug 1, 2022

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.

@PeerRich PeerRich requested a review from krumware August 1, 2022 20:56
@PeerRich
Copy link
Member

PeerRich commented Aug 1, 2022

wow what a huge refactor! amazing. Whats missing to go from draft to ready for review?

@PeerRich
Copy link
Member

PeerRich commented Aug 1, 2022

cc @krumware and @zomars

@Leopere
Copy link
Contributor Author

Leopere commented Aug 1, 2022

@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.

Copy link
Member

@zomars zomars left a 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.

@Leopere
Copy link
Contributor Author

Leopere commented Aug 1, 2022

@zomars, I mean, if you aren't the docker guru, that's almost better if it works for you, then I hit my target.

@krumware
Copy link
Member

krumware commented Aug 2, 2022

@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

please squash this commit before merge
@Leopere
Copy link
Contributor Author

Leopere commented Aug 2, 2022

@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.
@zomars
Copy link
Member

zomars commented Aug 2, 2022

@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?

@leog
Copy link

leog commented Aug 2, 2022

@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

@zomars
Copy link
Member

zomars commented Aug 2, 2022

No need for removal. If the db has a user already the wizard won't show anyways

@Leopere
Copy link
Contributor Author

Leopere commented Aug 2, 2022

@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

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.

@leog
Copy link

leog commented Aug 2, 2022

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!

@Leopere
Copy link
Contributor Author

Leopere commented Aug 2, 2022

Also, for added clarity, there are two modes for the actual boot invocation of the app itself. Invoked from this case switch here the start.sh script's preflight checks determine what boot mode is to be used.

bootstrap_start is a function that was added for first-time operation in a deployment where it simply detects if there is a package.json inside of the $APP_PATH, and if it doesn't detect $APP_PATH/package.json, it will simply clone and compile the exact git commit requested from the specifically requested upstream git repository. Then it automatically runs the following boot function, basic_start, where it starts the app as defined in the previous docker container iteration.

basic_start simply changes the working directory into the $APP_PATH and executes the former containers' start functionality.

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.

@Leopere
Copy link
Contributor Author

Leopere commented Aug 2, 2022

Another detail that was brought up, and I will adjust the README.md and docker-compose.yml to accommodate, is that its no longer required to deploy with Prisma Studio as there is now a first install wizard. This said further lines of questions may need to be answered before this is added. Perhaps if this PR gets accepted, a new TODO list item will need to be created in lieu of this PR's acceptance.

@Leopere
Copy link
Contributor Author

Leopere commented Aug 2, 2022

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 basic_start is effectively identical to the original container, and now with far fewer container layers, the initial pull speed should be lightning fast.

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 https/tls and the cryptographically signed byte-perfect git fetch feature.

README.md Outdated Show resolved Hide resolved
@@ -0,0 +1,103 @@
# Use postgres/example user/password credentials
Copy link

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.

Copy link
Contributor Author

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.

Copy link

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.

Copy link

@avinal avinal left a 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]>
@Leopere
Copy link
Contributor Author

Leopere commented Aug 10, 2022

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 docker compose tend currently to already be wise to the change and lack of need to use the hyphenated version that's installed separately.

Copy link
Member

@PeerRich PeerRich left a 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 🙏

@Leopere
Copy link
Contributor Author

Leopere commented Aug 16, 2022

Well I'd still really like to get this working or even help elsewhere.

@gregwbrooks
Copy link

@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?

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