search

mautic / docker-mautic

Docker Image for Mautic
https://www.mautic.org
374 stars 278 forks source link

Mautic 4 container updates #196

Closed thinkl33t closed 3 years ago

thinkl33t commented 3 years ago

Incorporates:

Adds:

Closes: #173

luizeof commented 3 years ago

Hello @thinkl33t we are going to use the mautic 4 branch, could you check if this pull is compatible?

thinkl33t commented 3 years ago

Hello @thinkl33t we are going to use the mautic 4 branch, could you check if this pull is compatible?

We're currently using this on mautic 4.0.0-rc internally.

RCheesley commented 3 years ago

@luizeof let's get this merged, it has been waiting for far too long and we need to move forward. Sorry for all the delays @thinkl33t.

luizeof commented 3 years ago

Hello @thinkl33t, great job on this pull request !!

I did many tests with your proposal over the weekend and everything went fine, I didn't find any errors and I believe we can merge with the branch of mautic4.

But there are 2 points we should talk about to avoid some problems, which we usually cover in our internal image here at Powertic and we even use it with other systems as well:

Automatically apply migrations:

Many people use low-resource mautic, often use VPS on popular low-memory providers, and cause MySQL to be overloaded and lacking resources on the machine, causing some commands to run at specific times.

In the case of large databases, it can cause the container not to start, even though it can be started, as it can take a long time until the ALTER TABLE migrations are executed, or even indexes are created, according to the number of processes running on mysql.

We also use mautic to scale on AWS Fargate or any other scaling system like swarm, etc. and in these cases at each new instance we will try to apply migrations that are outside the scope of the container, which is scaled to receive HTTP requests. You can see that there is an ENV to not run cron jobs in these cases.

I think we can apply an ENV and let the user choose to run or not this command automatically.

Automatic Setup

Whenever we put things on automatic, it's always good to check and validate user input, as I believe many newbies in mautic will get their hands on these containers.

In Auto Setup, it would be good to validate the url and email, even if it's a simple format validation, so that an error doesn't advance to the rest of the installation process.

I believe we can discuss whether it would be good to fail with the setup if the user specified some invalid value in the email or url, in order to guarantee the consistency of the setup.

thinkl33t commented 3 years ago

Thanks for the review @luizeof!

I'd suggest we:

1) add the environment variable for disabling automatic migrations as part of this PR, as it is a fairly easy fix 2) we split out the sanity checking of provided URLs and email addresses into a separate issue + PR once this is merged - its a bit more complicated and would be easier to review as a standalone PR

Does that sound acceptable?

thinkl33t commented 3 years ago

I've added the ability to disable migrations from automatically running as requested.

thinkl33t commented 3 years ago

It looks like the admin email is already checked by the installer itself to confirm it is a valid email address:

mautic_1  | Parsing options and arguments...
mautic_1  | 0 - Checking installation requirements...
mautic_1  | Missing optional settings:
mautic_1  |   - [0] It is recommended to secure your installation with an SSL certificate (https). Starting February 2020 Google Chrome will stop sending third-party cookies in cross-site requests unless the cookies are secure. Tracking will stop working for Mautic instances running on HTTP. Use HTTPS.
mautic_1  | Ready to Install!
mautic_1  | 1 - Creating database...
mautic_1  | 1.1 - Creating schema...
mautic_1  | 1.2 - Loading fixtures...
mautic_1  | 2 - Creating admin user...
mautic_1  | Errors in admin user configuration/installation:
mautic_1  |   - [0] A valid email is required.
mautic_1  | Install canceled

This puts the container into a restart-loop as the installer stops running here, which is typical behaviour in this kind of situation.

What are you thinking for verifying the URL? Given the number of different ways a URL can be specified, i'd suspect that the easiest way would be attempting a curl to it, but it is possible that will fail when running behind a reverse proxy.

mautibot commented 3 years ago

This pull request has been mentioned on Mautic Community Forums. There might be relevant details there:

https://forum.mautic.org/t/getting-ready-for-mautic-4-all-details-timelines/18692/16

luizeof commented 3 years ago

@thinkl33t thanks for your updates.

I'm testing some scenarios and running the setup using the default settings is generating this error:

`

Mautic not currently installed (no secret_key in local.php)

URL & Admin credentials supplied, attempting automated mautic installation.

Mautic Install

Parsing options and arguments... 0 - Checking installation requirements... Missing optional settings:

======================================================================== Applying any needed database migrations

[2021-07-20 14:11:14] ++ adding generated column generated_sent_date [2021-07-20 14:11:14] -> ALTER TABLE email_stats ADD generated_sent_date DATE AS (CONCAT(YEAR(date_sent), '-', LPAD(MONTH(date_sent), 2, '0'), '-', LPAD(DAY(date_sent), 2, '0'))) COMMENT '(DC2Type:generated)'; ALTER TABLE email_stats ADD INDEX generated_sent_date_email_id(generated_sent_date, email_id)

In AbstractMySQLDriver.php line 61:

An exception occurred while executing 'ALTER TABLE email_stats ADD generate
d_sent_date DATE AS (CONCAT(YEAR(date_sent), '-', LPAD(MONTH(date_sent), 2,
'0'), '-', LPAD(DAY(date_sent), 2, '0'))) COMMENT '(DC2Type:generated)';
ALTER TABLE email_stats ADD INDEX generated_sent_date_email_id (generated_sent_date, email_id)':

SQLSTATE[42S02]: Base table or view not found: 1146 Table 'm4zerado.email_s
tats' doesn't exist

In Exception.php line 18:

SQLSTATE[42S02]: Base table or view not found: 1146 Table 'm4zerado.email_s
tats' doesn't exist

In PDOConnection.php line 132:

SQLSTATE[42S02]: Base table or view not found: 1146 Table 'm4zerado.email_s
tats' doesn't exist

doctrine:migrations:migrate [--write-sql [WRITE-SQL]] [--dry-run] [--query-time] [--allow-no-migration] [--all-or-nothing [ALL-OR-NOTHING]] [--configuration [CONFIGURATION]] [--db-configuration [DB-CONFIGURATION]] [--db DB] [--em EM] [--shard SHARD] [-h|--help] [-q|--quiet] [-v|vv|vvv|--verbose] [-V|--version] [--ansi] [--no-ansi] [-n|--no-interaction] [-e|--env ENV] [--no-debug] [--] []

Terminal will be reused by tasks, press any key to close it. `

luizeof commented 3 years ago

@thinkl33t another point is that ENV MAUTIC_RUN_MIGRATIONS true should by default be set to false, as it is optional, true by default will run anyway.

The same happens with the installation envs MAUTIC_ADMIN_EMAIL, MAUTIC_ADMIN_PASSWORD, MAUTIC_URL, existing by default will always invoke automatic installation.

I think the good thing is by default to let the user go through the setup, especially since it's the default for mautic. If he wants a facility, he fills the envs and then the setup happens automatically.

I believe that @RCheesley can help us with this decision, as users of various skill levels will use these containers.

iBobik commented 3 years ago

On top of it there is also stateless version (Docker best practice) of the image: https://github.com/mautic/docker-mautic/pull/198

luizeof commented 3 years ago

hello @thinkl33t how are you ? hope it's ok ;) could you see the items i commented above ? See you.

RCheesley commented 3 years ago

Hi folks, can I get an update on this? @kuzmany I think that some of the team at Webmecanik were reviewing this PR but I do not see their reviews in the comments. Can you ask them to add their feedback please?

Also @thinkl33t it looks like we have some conflicts to address.

@iBobik I believe the idea was to get these changes merged and then move on to looking at the stateless PR (which is still marked as WIP)

thinkl33t commented 3 years ago

I no longer have any dedicated time allocated to this project through work.

If someone else wants to take over this PR to get it across the line and merged please feel free. I might be able to carve out an afternoon sometime this month but i'm not 100% confident of that.

iBobik commented 3 years ago

I currently my fork (your stateless version with upgraded to 4.0.0-rc) and would like to upgrade it further. Also I can fix Docker issues when I will face some.

Do you at least use some of this images, so you can confirm it works in your case?

Honza Pobořil

    1. 2021 11:17:02, Bob Clough @.***> napsal:

I no longer have any dedicated time allocated to this project through work.

If someone else wants to take over this PR to get it across the line and merged please feel free. I might be able to carve out an afternoon sometime this month but i'm not 100% confident of that.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mautic/docker-mautic/pull/196#issuecomment-896651012, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEV6WASZXJEXC766UN6QOTT4I5Y5ANCNFSM45JLYUZQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email .

thinkl33t commented 3 years ago
luizeof commented 3 years ago

hello @RCheesley how are you? I was still testing the fixes that were uploaded from pull 196 and saw that you merged. I have limited time, but as I've told you before, I always work at the mautic-docker on Fridays.

i have been waiting for dockerhub release to use the mautic4 branch with dennis contributions.

The contributions of this pull are fantastic, but some testing of the following scenarios is still pending:

I do several tests before publishing, as there are many scenarios that over the years we've been discovering that can affect the build.

Did you actually test it before merging this pull?

rvalle commented 3 years ago

@luizeof @RCheesley Perhaps an v4-beta could be pushed to docker hub for community feedback?

francoisauclair911 commented 1 year ago

Is there any update on this ?