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

DDEV project config #22245

Draft
wants to merge 7 commits into
base: 5.x-dev
Choose a base branch
from
Draft

DDEV project config #22245

wants to merge 7 commits into from

Conversation

michalkleiner
Copy link
Contributor

Description

Provides DDEV project base and resolves #21883
Replacing community PR #21884 so that it's easier for us to iterate on the work, rebase, test etc.

Review

@michalkleiner

This comment was marked as outdated.

@michalkleiner michalkleiner mentioned this pull request May 23, 2024
11 tasks
@sgiehl
Copy link
Member

sgiehl commented May 23, 2024

I haven't tested anything yet, but here are already some thoughts I had while looking at the code:

  • If we want to have the ddev config part of this repo, we need to ensure those files are removed before building a release
  • Assuming that the actual code is mounted into the vm: Running npm install will fail on windows in this case, as directory depth is kind of always too deep for the file system and in many cases it tries to create symlinks, which isn't supported on windows. A possible workaround is to hold the node_module folders fully inside the vm by e.g. overwriting the path with internal mount points or similar.

@michalkleiner michalkleiner force-pushed the dev-18030-ddev-env-config branch from eb3388b to 70f8381 Compare June 7, 2024 06:07
@michalkleiner
Copy link
Contributor Author

michalkleiner commented Jun 7, 2024

Ok, I think this is now in a testable state with some documentation needed.
Would someone be keen to check out this branch and test if these steps work?

  1. clone this repo, cd to the repo folder and checkout dev-18030-ddev-env-config branch
  2. (optional) run ddev config --project-name my-matomo to adjust the default project name (from matomo) and change all occurrences of matomo.ddev.site to my-matomo.ddev.site
  3. run ddev start to start the project up
  4. run ddev launch to open the instance in browser
  5. follow the installation process (db details should be pre-filled) and log in
  6. run ddev matomo:init:tests and see if all the install steps work
  7. run ddev matomo:console tests:run to see if tests work
  8. run ddev matomo:console tests:run-ui to see if UI tests work

@@ -0,0 +1,41 @@
#!/bin/bash
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#!/bin/bash
#!/usr/bin/env bash

For all the host commands.

My NixOS box only has /bin/sh and /usr/bin/env available, no /bin/bash. I would not expect someone to have this problem, but /usr/bin/env should be just a tiny bit more portable.

We could also switch to plain /bin/sh, but that may require changes to the read arguments afaik.

Comment on lines +16 to +19
- exec: MYSQL_PWD=root mysql -uroot -e "CREATE DATABASE IF NOT EXISTS matomo_tests; GRANT ALL ON matomo_tests.* to 'db'@'%';"
service: db
- exec: MYSQL_PWD=root mysql -uroot -e "CREATE DATABASE IF NOT EXISTS _UITestFixture; GRANT ALL ON _UITestFixture.* to 'db'@'%';"
service: db
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- exec: MYSQL_PWD=root mysql -uroot -e "CREATE DATABASE IF NOT EXISTS matomo_tests; GRANT ALL ON matomo_tests.* to 'db'@'%';"
service: db
- exec: MYSQL_PWD=root mysql -uroot -e "CREATE DATABASE IF NOT EXISTS _UITestFixture; GRANT ALL ON _UITestFixture.* to 'db'@'%';"
service: db
- exec: MYSQL_PWD=root mysql -uroot -e "GRANT ALL PRIVILEGES ON *.* to 'db'@'%'"
service: db
- exec: MYSQL_PWD=root mysql -uroot -e "GRANT ALL PRIVILEGES ON *.* to 'db'@'localhost'"
service: db

This would allow us to configure db:db for the tests instead of root:root.

Otherwise there may be databases not visible to the db user, for example when running when running the EmptySite spec:

ddev matomo:console tests:run-ui --persist-fixture-data EmptySite

Without more privileges the created _EmptySite database would not be visible.

Not 100% sure if we need to run the statement for if for @localhost, I think it was required to have both ddev mysql and mysql to have all the privileges..

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 wouldn't expect the @localhost to be needed as the db is not running on the same system so PHP will never access it on localhost.

I'll test it out, thanks for pointing this out and identifying a test that can be impacted by this.

- exec: MYSQL_PWD=root mysql -uroot -e "CREATE DATABASE IF NOT EXISTS _UITestFixture; GRANT ALL ON _UITestFixture.* to 'db'@'%';"
service: db
- exec-host: ddev matomo:init
webimage_extra_packages: [build-essential, chromium, fonts-dejavu, fonts-liberation, gconf-service, imagemagick, imagemagick-doc, libappindicator1, libasound2, libatk1.0-0, libcairo2, libdrm2, libgbm1, libgconf-2-4, libgdk-pixbuf2.0-0, libgtk-3-0, libnspr4, libnss3, libpango-1.0-0, libpangocairo-1.0-0, libx11-xcb1, libxcomposite1, libxcursor1, libxdamage1, libxfixes3, libxi6, libxrandr2, libxrender1, libxshmfence1, libxss1, libxtst6, ttf-mscorefonts-installer, xdg-utils]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of the screenshot tests give really weird results. PhpStorm shows all processed screenshots as "24-bit" instead of "32-bit".

And some screenshots created in the Dashboard spec (e.g. Dashboard_dashboard1_mobile.png) are stretched beyond recognition, though the height/width of the file match the expected screenshots.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

24 vs 32 bit wouldn't be a problem if we eventually run CI using ddev as well, as long as the images carry the visual information they should.

Could you please send me some of the image names or even some of the files so that I could compare them with mine?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sent you my local images and some additional information. It looks like the DDEV environment generates images without an alpha channel:

// repository
  Type: TrueColorAlpha
  Endianness: Undefined
  Depth: 8-bit
  Channels: 4.0
  Channel depth:
    Red: 8-bit
    Green: 8-bit
    Blue: 8-bit
    Alpha: 1-bit

// ddev
  Type: TrueColor
  Endianness: Undefined
  Depth: 8-bit
  Channels: 3.0
  Channel depth:
    Red: 8-bit
    Green: 8-bit
    Blue: 8-bit

@rr-it
Copy link
Contributor

rr-it commented Jul 1, 2024

I had some issue with starting the web-container caused by supervisord: PermissionError: [Errno 13] Permission denied: '/var/log/supervisord.log'
Also see ddev/ddev#4899

Adding this file .ddev/web-entrypoint.d/fix-var-log-directory-permissions.sh solved it:

#!/bin/bash

# Make web-container `/var/log` directory writable for supervisord.
sudo chmod 777 /var/log

Copy link
Contributor

If you don't want this PR to be closed automatically in 28 days then you need to assign the label 'Do not close'.

@github-actions github-actions bot added the Stale The label used by the Close Stale Issues action label Jul 16, 2024
Copy link
Contributor

This PR was last updated more than one month ago, maybe it's time to close it. Please check if there is anything we still can do or close this PR. ping @matomo-org/core-reviewers

@github-actions github-actions bot added the Stale for long The label used by the Close Stale Issues action label Aug 28, 2024
@michalkleiner michalkleiner added the Do not close PRs with this label won't be marked as stale by the Close Stale Issues action label Nov 26, 2024
@michalkleiner michalkleiner force-pushed the dev-18030-ddev-env-config branch from 70f8381 to 1845c9f Compare December 19, 2024 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Do not close PRs with this label won't be marked as stale by the Close Stale Issues action Stale for long The label used by the Close Stale Issues action Stale The label used by the Close Stale Issues action
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dev: provide DDEV project config for new developers
4 participants