-
Notifications
You must be signed in to change notification settings - Fork 25
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
The good, the bad and the ugly: We need a Docker expert to guide us! #31
Comments
Glancing through some of the above issues/PRs, it looks like the current docker image works for specific configurations (correct me if I'm wrong on that), but it's missing bits and pieces to cover all of the bases. It's based on alpine, so I have to ask how important image size is, and whether or not the aim is to build an all-in-one image (or even docker-compose configuration) or just a "core service" (fpm/nginx/cypht) that can be connected to external services (mysql/postgresql/redis/etc...)? I'd be interested to help (my end goal is a helm chart to ease kubernetes deployments, so having a solid docker image is a blocker for me), and I have a lot of experience with Docker, but I hesitate to call myself an "expert" in the field. Also, is this repo being archived and the configs moved to the main cypht development repo, or is the intent to get this repo brought up to date? |
That is my general understanding as well, but I don't use it myself. (I use Cypht via Tiki: https://doc.tiki.org/Cypht)
That is one of the things a Docker expert would advise us on. We need to determine the potential use cases (production, development, CI, etc.) and what we can realistically maintain.
That is one of the things a Docker expert would advise us on. But in previous discussions, the conclusion was to have it in the main repo, and shut this one down. |
I'm working on an updated Dockerfile, definitely won't resolve all of the issues/PRs above, but I'll see how many I can knock out in a single go. For now, I'll put the PR in this repo in a few when it is ready. In some ways it makes sense to have the "Docker/helm" stuff separate from the codebase, but I don't see why it couldn't be integrated into the main repo (that would necessitate some additional changes to the build, but that's simple enough). |
To provide an update, I've got an updated build with postgresql drivers added with a lot of tweaks to the docker-entrypoint.sh script and I'm currently investigating why database tables are not being added correctly. Once I get a working docker-compose.yaml setup going I'll submit a PR and start fixing the remaining issues. |
Hello team, unfortunately I will have to bow out of the development of an updated docker image. I have a medical issue that has come up requiring my full attention over the next few months. I have a partial build (IIRC, the last part I was investigating was the reason for database drivers not loading properly) that I will gladly PR in the event it is deemed worthy to use moving forward. My apologies for being unable to dedicate sufficient time to this task. |
@rocket357 Sorry to hear and best of luck + focus for what is ahead. Please do push the PR of the partial build as it will serve as inspiration to others. |
Related: |
I may be able to help with some Docker things. I noticed some of the above are unmerged PRs like this one: Are you waiting for a Docker expert to test and review those? |
Yes please. The biggest one is cypht-org/cypht#975 It is done on the main repo because it was discussed/concluded in a meeting a while back that Docker-related code should be be in a distinct repo. I don't know enough about Docker to agree or disagree. |
I'm a little confused. What was the decision and where can I see the discussion? |
It was a videocall. I don't think it was recorded. Yes, the idea was to move everything to the main cypht repo. |
Ok. I have done a high level look at things here and have some questions/suggestions about the current docker setup. Consider this a "fresh eyes" view since I dont know the history and have not dived into the code much. I have dockerized quite a few apps before, though not much in the php world so I need to look up best practices with fpm and the like. If I were to dockerize cypht from scratch here are some things I would do... I do agree the Dockerfile should be in main repo but not in a hidden dir like '.github'. By the sound of things docker is already a first class delivery method for cypht, so it should be given first class treatment in a subdirectory or right in the root of the main repo. github would only be one of its uses. Anyone should be able to quickly run a local docker build. This would be particularly useful for development and testing. I very much like the use of env vars, but how about keeping the same env vars used by the app as used by docker.This means getting rid of vars like CYPHY_SESSION_TYPE. If you just use SESSION_TYPE it looks like that env var is already read by config/app.php. Then you can get rid of most of the special handling of vars in docker-entrypoint.sh . That would also help with the .env file. Docker has built in support for env files, so ideally the same env file should be usable inside and outside the container. If the var names are the same, the env file would be portable. These would simplify the docker setup and make containers easier to support. I can show how these things work if this is a direction yall are interested in. |
Sorry for the confusion, the Dockerfile is in the main directory, but I put its additional scripts in .github because they are not beautiful in the main directory. In my current PR, the docker script is indeed very messy. But I just copied the original script, and I want to run it in the whole workflow first. Then I will start to optimize it. If you are interested, you can join us. At present, most of the processes can work normally. But there are improvements to be made, such as the environment variables you mentioned, such as reducing the size of the container. My Docker experience is very limited, and I have to check the documentation when I encounter problems. I need your help. Let's PR together. 😋😋 |
Yes, I'm happy to PR together. I'll will whip up a basic proof of concept for relocating the directory. Also, looks like you are a Nix user. I am too and that kind of what brought me to the cypht issue tracker. So we can talk on that on the side. :-) |
Here is the local directory POC: cypht-org/cypht#980 |
Another thing I realized. Docker images should be tagged with version numbers. It can probably be automated such that when a tag is created in github, an image is built and tagged in dockerhub. Since people should/will want to know what version they are running. |
Currently this container image is only a nightly version. It is just for testing demo. |
What I expect people will want:
|
Here is Cypht lifecycle: https://github.com/cypht-org/cypht/wiki/Lifecycle |
Thanks for the info. This is not semantic versioning though. It sounds more like a variation on time based versioning. Probably better discussed in another thread. |
Good point: cypht-org/cypht#982 |
@jonocodes wrote:
@wangxiaoerYah wrote
So I suppose we follow @jonocodes's lead because he has more experience? |
Here is my work in progress if anyone wants to follow along/help: |
For more info on the new Docker builder and how we will migrate, please see: cypht-org/cypht#1047 Thank you @jonocodes for your leadership and expertise. Thank you @wangxiaoerYah for your contributions. @rocket357 I hope things improve for you. The Helm chart is a logical next step. |
Low priority follow-up: cypht-org/cypht#1146 |
Spin off: cypht-org/cypht#1175 |
The good
Cypht currently has a record number of contributors: https://openhub.net/p/cypht/contributors/summary
We have:
We are building fantastic things on top of the fantastic work that Jason Munro did for many many years.
The bad
The ugly
The ask
So this is a call to Docker experts out there. We need you!
We don't need you to take this on forever. 2 PHP developers will work with you on the project. We need an expert to review the overall situation, chart a plan, and lead/guide/coach our developers.
Thanks!
The text was updated successfully, but these errors were encountered: