-
Notifications
You must be signed in to change notification settings - Fork 163
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
Docker refresh #1001
Docker refresh #1001
Conversation
I am thrilled by the progress here! |
Great. Once I get answers to some of my discussion questions (mostly about php 8) I think we can start testing it. This is currently setup to push images to dockerhub. For now thats my account. Should we do this on a more official cypht dockerhub account or use a different registry like github? I'm asking because we probably want to automate the building and pushing when a tag is released. Then we can use the creds for the offical/shared account. And what do you think @wangxiaoerYah ? |
Good point! Spinning off the discussion to: #1016 |
I totally agree, I think our review process needs to be accelerated to cope with everyone's time difference. It should be fast-forwarded, at least on Docker, people need a working container. |
I feel that anyway docker is always one of the best distribution methods at the moment, we need to decide it as soon as possible, and should not discuss it again and again. Just do it. The community contributors are currently less active, and it is obviously not a good thing to ask them to review, they have not commented and reviewed for a long time. Can I merge? |
I dont recommend merging this yet. I have experimental changes in it including changes to php code. This is intended to be a long term solution. It is almost ready though. Even if it gets merged, it wont be usable. It needs documentation on how to use it and we need to figure out the last mile - how/where the images get published to. In the short term there are other things we can do. I dont know the story of the urgency, but if the concern is people are pulling an outdated image, I would suggest using the current cypht/cypht-docker repo to pull in more recent code and push to the current sailfrog/cypht-docker:latest repo. Sounds like we have access to that dockerhub account. I think either of us should be able to get that figured out. |
@@ -132,6 +132,8 @@ public function create($user, $pass) { | |||
$result = 0; | |||
$res = Hm_DB::execute($this->dbh, 'select username from hm_user where username = ?', [$user]); | |||
if (!empty($res)) { | |||
// TODO: send this to 'debug' once I figure out how it works | |||
print("user {$user} already exists\n"); |
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.
Hm_Debug::add?
You should also check DEBUG_MODE constant - it is true in index.php but config_gen copies that to site/index.php (which is used as the main entry point of the web app) and turns it into false. Keep it to true in docker env if you want deubgging mode turned on or make it come from dotenv.
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 dont see where DEBUG_MODE is read in the environment. I only see it hard coded at the beginning of scripts. So I cant pass it in. I'm confused what DEBUG_MODE is for exactly, but I dont think it does what I'm going for. It looks like a way of enabling features and it does some log queuing.
I was looking for a way to log at different severity levels. I'm not sure of the php way to do that.
I am just trying to print a message to the console at the time it happens. So I think print, or error_log seem ok for here.
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.
Yes, DEBUG_MODE is only hard-coded in scripts for now. It will be a good idea to extend that to come from the environment.
It is best to still use the debug class to queue these messages. Hm_Msgs is another one meant to be displayed to the user. Console scripts can dump the debug and/or msgs arrays at the end to the cli using print statements. error_log will send the message to the php error log which seldom appears on the screen for users (depends on several PHP settings to appear).
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 am genuinely confused with the logging setup and it has bitten me several times. There is a lot of good info being sent go Hm_Debug::add, but its hard to get the data out from there. I lost several hours trying to fix a sqlite bug that would have been minutes if those logs were simply displayed.
Its possible PHP logging is different because one of the output streams is meant to screen/browser output. I hear Laraval is a commonly used PHP framework, so I looked up their logging setup: https://laravel.com/docs/11.x/logging
which seems quite standard. It uses monolog which provides simple, immediate, customizable outputs.
Let me know if I am off base, or I can create a separate ticket for this and elaborate more.
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.
Hm_Debug uses Hm_list which has get
method to get the messages and show
method to send them to the error log. PHP error logging might be complex and not everything logged, indeed, it has settings for error_reporting - what kind of errors to report, then displaying errors on screen or writing errors to a log file - thus third option also depends on your backend server setup. So, a lot of moving parts.
Integrating something like monolog is an option but note that cypht started with minimalism in mind while Laravel is enormous, so we still try to depend on fewer external libs. That being said, I think an ENV setting for DEBUG_MODE which is observed and makes sure debugging messages are written to the php error log at the end of a request + screen/cli if necessary seems like a better approach to me.
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 moving this discussion to a separate issue #1027
We can automate the building and pushing to dockerhub, cypht has a dockerhub account. We can automate the building and pushing using github actions. @jonocodes is there a particular reason to publish to github registry instead of dockerhub ? Thanks! |
@jonocodes there are 5 months, I made the last push to sailfrog/cyph-docker building multiarch image after cypht v1.4.1 has been release. I was waiting for cypht-1.4.2 to be released to push a new tag to respect the versioning having for each release a corresponding tag. If you are worried about people pulling an outdated image and waiting for your MR to be ready, I can build and publish an image with the code of cypht v1.4.3 with cypht-org/cypht-docker. |
Dockerhub sounds good. Does not matter to me where the images go. |
I myself am not worried about this, but you may as well update that image. Thanks. |
@jonocodes did you try to send an email with an attachment ? I tried but without success, the email is sent but the attachment is not received. |
No, I am running into other problems though. I cant even add an IMAP account but I have the same issue in master without docker, so I cant quite test this now. I think it's an ssl issue but have not had time to dig in.
Side question. Is there something like a smoke test document or procedure that is followed before release? It may be a list of features to try to make sure basic things have not broken. For example it may say:
|
It is on the todo: #1031 |
Here are the details:
About something like a smoke test, @marclaporte already answered the question. |
Ok, I was able to recreate the issue and fix it. Pull this branch and try again. |
I pulled this branch and tested again, the issue has been fixed, the attachment is sent and received. Thanks! |
Ok, I removed the "WIP" from the title, and I think its probably good enough to merge. I think it already has fewer kinks then the original image. Here is how I imagine rolling this out... Once approved and merged one of you guys with the sailfrog dockerhub creds use the make recipe to manually publish to sailfrog/cypht-docker:latest Once the next release is ready (ie 2.1.0), we push to sailfrog/cypht-docker:2.1.0 manually. After that we decide how to work this into a github action/automation for the next release perhaps. And how to maintain 'latest' and other non-versioned tags. |
Thanks @jonocodes for the work, but there is an update about where cypht docker image will live. We decided to create an organization account with the name |
Alea iacta est |
We now have version 2.1.0 tagged: https://hub.docker.com/r/cypht/cypht/tags |
Ok, let's collaborate here: #1175 |
Goal
The primary goal is to simplify the docker setup and make it as similar as possible to the local development setup. Some of the changes include:
Running the official image
This uses the image from dockerhub.
See setup instructions here: https://hub.docker.com/r/jonocodes/cypht
or use the local docker compose file:
Then visit http://localhost
Running docker for development
This uses the the current checked out code in a docker container for development. You can edit source files while it running:
Then visit http://localhost
Status