search

moodlehq / moodle-docker

A docker environment for moodle developers
GNU General Public License v3.0
373 stars 244 forks source link

Added option to include adminer or phpmyadmin. #236

Closed michael-milette closed 1 year ago

michael-milette commented 1 year ago

This fix addresses the feature request described in issue #235 . It gives you the option of adding Adminer or phpMyAdmin.

To use, just export MOODLE_DOCKER_DB_MANAGER= either adminer or phpmyadmin and start your containers as usual. The selected tool will be available on http://localhost:8900. If this environment variable is not set, neither of these tools will be installed.

You can optionally change the port number by setting MOODLE_DOCKER_DB_MANAGER_PORT to a different number.

For more information, see README.md. As noted in the documentation, while Adminer supports all databases, phpMyAdmin is only for MySQL and MariaDB.

Let me know if you have any questions or concerns.

Best regards,

Michael Milette

scara commented 1 year ago

Hi @michael-milette, we usually set the image version for consistency during development and testing i.e.:

These are tools absolutely not related with the Moodle code and its dependencies i.e. they could not obey to the rule above: for your consideration - you could even add an optional MOODLE_DOCKER_DB_MANAGER_VERSION.

TIA, Matteo

michael-milette commented 1 year ago

Hi @scara

I am not sure what you mean. Are you suggesting that I add a MOODLE_DOCKER_DB_MANAGER_VERSION setting, or are you saying that you are not interested in this contribution?

Best regards,

Michael

scara commented 1 year ago

Hi @michael-milette, I mean, the image element for both the DB Managers are missing the tag (version) in your proposal: did you do that on purpose or we can set a <Major.minor> tag even for both the DB Manager images? What is your thought?

Mine was a quick glance PR given the "coding style" actually used in the other Docker Compose services 😉. E.g.: moodlehq/moodle-exttests has no version since the tests are w/o version in https://github.com/moodlehq/moodle-exttest.

HTH, Matteo

michael-milette commented 1 year ago

Thanks for the feedback and clarification @scara . It helped. :-)

I added the version tags for both Adminer and phpMyAdmin as you suggested.

Best regards,

Michael

scara commented 1 year ago

Hi @michael-milette,

It helped. :-)

Glad for it! I'm not English native and I can be easily misunderstood 😉.

Another round on top of your new https://github.com/moodlehq/moodle-docker/pull/236/commits/5f5cbc9a5799d1f07d21b7076ec02b834189eba3. My suggestion was to stop to the firsts (two) version numbers given that a more general tag exists:

Doing so, the user can benefit of any security release - if she/he drops the local image first! - thanks to the (kind of) semantic versioning of that tag and we do not need to update the Moodle Docker Toolbox to avoid security issues in the DB Managers - even if this toolbox is for local development. I'm asking to the Maintainers for opinions here: @stronk7, any thought here?

My position:

HTH, Matteo

andrewnicols commented 1 year ago

See #242 for an example of how this could instead be solved.

stronk7 commented 1 year ago

I'm closing this in favour of https://github.com/moodlehq/moodle-docker/pull/242 that seems to be the correct way to go.