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

Use adminerevo docker image instead of the deprecated adminer image, fixes #27 #30

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

M-arcus
Copy link

@M-arcus M-arcus commented Nov 21, 2024

The Issue

How This PR Solves The Issue

It uses the adminerevo docker image

Manual Testing Instructions

For DDEV v1.23.5 or above run

ddev add-on get https://github.com/M-arcus/ddev-adminer/tarball/adminerevo

For earlier versions of DDEV run

ddev get https://github.com/M-arcus/ddev-adminer/tarball/adminerevo

Afterwards run ddev restart

Then you can just ddev adminer or use ddev describe to get the URL (https://<project>.ddev.site:9101).

Automated Testing Overview

See the GitHub Action https://github.com/M-arcus/ddev-adminer/actions/runs/11956882733

Related Issue Link(s)

Release/Deployment Notes

  • Does the readme need to be rewritten because of the name?
  • Is the choice of the docker image correct?

@rfay rfay changed the title Add adminerevo Use adminerevo docker image instead of the deprecated adminer image Nov 21, 2024
@rfay rfay changed the title Use adminerevo docker image instead of the deprecated adminer image Use adminerevo docker image instead of the deprecated adminer image, fixes #27 Nov 21, 2024
@rfay
Copy link
Member

rfay commented Nov 21, 2024

Your testing instructions are not correct as they point to a release on your fork.

Instead, they should be

ddev add-on get https://github.com/M-arcus/adminerevo/tarball/adminerevo

But in general, please don't use the branch main for a PR. That means that you can't ever change main on your fork.

And I don't think you'll want to be introducing the actual contents of your edited add-on, instead focus on the actual changes.

Maybe it would be best to create a new branch that focuses on being a PR?

@M-arcus
Copy link
Author

M-arcus commented Nov 21, 2024

This PR was created with the branch https://github.com/M-arcus/ddev-adminer/tree/adminerevo, not with main.
image

@M-arcus
Copy link
Author

M-arcus commented Nov 21, 2024

And I don't think you'll want to be introducing the actual contents of your edited add-on, instead focus on the actual changes.

What edited contents are meant?

@rfay
Copy link
Member

rfay commented Nov 21, 2024

Your manual testing instructions say to use ddev add-on get M-arcus/ddev-adminer

That will get a released version of the fork, not the current branch.

Sorry I didn't read your branch correctly, I edited my suggestion for the testing instructions, which will get your actual branch.

Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

The Dockerfile is obsolete, so you can remove the build section of the docker-compose.adminer.yaml and instead add an image statement.

README.md Outdated Show resolved Hide resolved
Comment on lines 1 to 2
#ddev-generated
FROM adminer:standalone
ADD ddev-login.php /var/www/html/plugins-enabled/
FROM shyim/adminerevo:latest
Copy link
Member

Choose a reason for hiding this comment

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

This file serves no purpose here (that I can understand). It should just be removed, and the docker-compose.adminer.yaml should use the upstream image.

Copy link
Author

Choose a reason for hiding this comment

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

I have removed the file, added the image to the docker-compose.adminer.yaml and updated the install.yaml

@M-arcus
Copy link
Author

M-arcus commented Nov 21, 2024

I have updated the manual testing instructions in the initial comment.

Co-authored-by: Randy Fay <randy@randyfay.com>
@rfay
Copy link
Member

rfay commented Nov 21, 2024

In its current state, after you add the image back into docker-compose.adminer.yaml, you can remove the adminerevo directory.

@rfay
Copy link
Member

rfay commented Nov 21, 2024

Are you confident that adminerevo is the most likely successor to adminer?

@rfay
Copy link
Member

rfay commented Nov 21, 2024

The tests (https://github.com/ddev/ddev-adminer/blob/main/tests/test.bats) look pretty limited, it's worth considering looking at them in this or a followup PR.

@M-arcus
Copy link
Author

M-arcus commented Nov 21, 2024

Are you confident that adminerevo is the most likely successor to adminer?

Yes, it is often recommended and supports a wide range of technologies.

@@ -4,18 +4,15 @@

## What is this?

This repository allows you to quickly install Adminer database manager into a [Ddev](https://ddev.readthedocs.io) project using just `ddev get ddev/ddev-adminer`.
This repository allows you to quickly install the Adminerevo fork of the adminer database manager into a [Ddev](https://ddev.readthedocs.io) project using just `ddev get ddev/ddev-adminer`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
This repository allows you to quickly install the Adminerevo fork of the adminer database manager into a [Ddev](https://ddev.readthedocs.io) project using just `ddev get ddev/ddev-adminer`.
This repository allows you to quickly install the [AdminerEvo](https://docs.adminerevo.org/) fork of the [adminer](https://www.adminer.org/) database manager into a [DDEV](https://ddev.readthedocs.io) project using just `ddev get ddev/ddev-adminer`.


* MySQL / MariaDB
* PostgreSQL
Adminer is available for MySQL, MariaDB, PostgreSQL, SQLite, MS SQL, Oracle, Elasticsearch and MongoDB.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Adminer is available for MySQL, MariaDB, PostgreSQL, SQLite, MS SQL, Oracle, Elasticsearch and MongoDB.
AdminerEvo is available for MySQL, MariaDB, PostgreSQL, SQLite, MS SQL, Oracle, Elasticsearch and MongoDB.

Copy link
Member

Choose a reason for hiding this comment

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

How about "Adminer works with"

Moving to mention of AdminerEvo may end up adding confusion. I know we're changing the technology behind this. Should we just mention that in the README and then still use regular adminer terminology?

Copy link
Collaborator

@bserem bserem Nov 21, 2024

Choose a reason for hiding this comment

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

I like that idea

environment:
# Use the line below to change the adminer theme.
# - ADMINER_DESIGN=nette
- ADMINER_DEFAULT_SERVER=db
- ADMINER_DEFAULT_USER=db
- ADMINER_DEFAULT_PASSWORD=db
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we set a predefined DB here?

@bserem
Copy link
Collaborator

bserem commented Nov 21, 2024

Hey @M-arcus , thanks for the initiative and the work on updating this! I honestly was unaware of the changes in the adminer world.

The new version of the add-on works ok for me.

I am only wondering if we can set a predefined DB.
It wasn't possible with the original version either but @wotnak solved this in #1

@rfay
Copy link
Member

rfay commented Nov 21, 2024

Oh, the usage of ddev get is outdated, should be ddev add-on get.

And normally we encourage people to use a .ddev/docker-compose.adminer_extras.yaml that they control rather than editing the docker-compose.aadminer.yaml. That way they profit from future changes to the add-on without having to think about re-applying edits.

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.

3 participants