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

Added option to include adminer or phpmyadmin. #236

Closed
wants to merge 1 commit into from

Conversation

michael-milette
Copy link

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
Copy link
Contributor

scara commented Oct 16, 2022

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

  • adminer:4.8 (today: 4.8.1)
  • phpmadmin:5.2 (today: 5.2.0)

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
Copy link
Author

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
Copy link
Contributor

scara commented Oct 17, 2022

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
Copy link
Author

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
Copy link
Contributor

scara commented Oct 17, 2022

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 5f5cbc9.
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:

  • if we leave a "mayor" tag, the experienced user is able to update to the "latest version" if she/he locally drops the image. This way of working already happens when a "new" DB Server version appears under the <Mayor>.<minor> versioning scheme and the user want to move to the new version... indeed they need to drop their local DB Server image
  • if we leave the 3-numbers version, the user has a fixed setup which is nice (consistency) but we should update sometime the version in case of known security issues with no pressure since, I guess, we do provide a Development&Testing toolbox and nothing to be exposed to the Internet
  • if we provide an optional ENV VAR, MOODLE_DOCKER_DB_MANAGER_VERSION with the above default values, the user can force the DB Managers versions w/o any need to run after a security issue in a DB Manager

HTH,
Matteo

@andrewnicols
Copy link
Contributor

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

@stronk7
Copy link
Member

stronk7 commented Dec 5, 2022

I'm closing this in favour of #242 that seems to be the correct way to go.

@stronk7 stronk7 closed this Dec 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants