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

Launch with ddev adminer instead of launcha -a #16

Closed
wants to merge 4 commits into from
Closed

Launch with ddev adminer instead of launcha -a #16

wants to merge 4 commits into from

Conversation

mejta
Copy link
Contributor

@mejta mejta commented Aug 23, 2023

The Issue

The current way to launch Adminer is via ddev launcha -a. It's different from how e.g. PHPMyAdmin is launched (ddev phpmyadmin).

How This PR Solves The Issue

PR changes launching the Adminer the same way like PHPMyAdmin:
ddev adminer

Manual Testing Instructions

Run ddev adminer

@tyler36
Copy link
Collaborator

tyler36 commented Aug 23, 2023

Thank you for the PR @mejta .

I believe the original idea with ddev launcha -a was to be similar to ddev launch -a which was used to launch PHPmyadmin.
Now that PHPmyadmin has been removed from the default DDEV install (from 1.22), it's probably a good time to discuss this.

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.

I like this and think it's a good idea, but it doesn't seem like it accomplishes anything to remove launcha. If you leave launcha in there at least for a year or so, it will help people to adapt.

@mejta
Copy link
Contributor Author

mejta commented Aug 24, 2023

@tyler36 Thank you for your comment. @rfay Thank you for the code review and the change suggestion. I put launcha back, so both commands are supported, but launcha is deprecated now. Please review the changes again. Thank you, and thank you for bringing ddev to live; it's a super-useful tool!

@rfay
Copy link
Member

rfay commented Aug 24, 2023

This can be tested manually with ddev get https://github.com/mejta/ddev-adminer/tarball/main

@rfay
Copy link
Member

rfay commented Aug 24, 2023

@mejta in the future, you'll be a lot happier creating a named branch for PRs. That way, the next time you create a PR (or need the main branch for something else) life will be a lot better.

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.

Looks good, tested manually and it works right. It's a lot better than the launcha approach. Let's remember to remove launcha in a few months?

@rfay rfay mentioned this pull request Aug 24, 2023
@rfay
Copy link
Member

rfay commented Aug 24, 2023

I opened

But... as I was writing it I thought maybe we'd be better to just go ahead and change ddev launcha so it tells people to use ddev adminer. That's what we did with ddev launch -p too...

@mejta
Copy link
Contributor Author

mejta commented Aug 27, 2023

Thank you, @rfay, for approving the request. Next time, I will create a separate branch for that. Also, I will add an event to remove launcha in February :)

@mejta
Copy link
Contributor Author

mejta commented Aug 27, 2023

@rfay I've created the separate pull request #18 that has all the changes from this pull request, but in addition, it is in a separate branch and adds an error message for launcha command, so the user knows how to use it. Feel free to use any of the pull requests.

@mejta mejta closed this Sep 4, 2023
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