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

Describe App #18

Closed
rickmanelius opened this issue Aug 25, 2017 · 9 comments
Closed

Describe App #18

rickmanelius opened this issue Aug 25, 2017 · 9 comments

Comments

@rickmanelius
Copy link
Contributor

What happened (or feature request):

  • Feature request

What you expected to happen:

End-users can access information from ddev describe.

@rickmanelius
Copy link
Contributor Author

rickmanelius commented Oct 23, 2017

Summarizing from #15 (comment).

  • Button to trigger this information is located here Basic App Browsing #15 (comment).
  • Clicking brings up a modal with the information contained within ddev describe
  • API response from the CLI is parsed and displayed.
  • URLs to services like Mailhog and PHPMyAdmin are clickable links.
  • A person using the UI is not on the command line and therefore we should discard command line details, such as login credentials from within the app.

@rfay
Copy link
Member

rfay commented Nov 16, 2017

@rickmanelius I disagree about discarding the db credentials (#18 (comment)). Those are there for anybody who is doing an install, and an install is a normal thing to do.

@rickmanelius
Copy link
Contributor Author

Hi @rfay. To be clear, I'm not suggesting we remove this section:

MySQL Credentials
-----------------
Username:     	db
Password:     	db
Database name:	db
Host:         	db
Port:         	3306

What I intended was the instructions to login to the mysql container directly. Specifically this portion:

To connect to mysql from your host machine, use port 32772 on 127.0.0.1.
For example: mysql --host=127.0.0.1 --port=32772 --user=db --password=db --database=db

The first portion is absolutely beneficial for doing a site install. The second is more for someone wanting to login to the DB directly and someone using the UI, I would argue, is more likely going to click the link to the phpMyAdmin service. Let me know if that addresses your concern of if you feel strongly that both sets of DB credentials should still be surfaced!

@rfay
Copy link
Member

rfay commented Nov 16, 2017

The most important part I was concerned about is db/db/db/db.

However... there's no reason that a ddev-ui user would be less likely to use the external credentials (using 127.0.0.1 and port number). I agree they might be less likely to use the mysql command line, but they're perfectly likely to use another tool (like the many competitors to sequelpro). So the problem is probably that we focus the external credential part on the mysql client command-line tool instead of general instructions.

@rfay
Copy link
Member

rfay commented Nov 16, 2017

But note @andrew-c-tran - the main credential section was not intended to be removed.

@andrew-c-tran
Copy link
Contributor

understood, the command line instructions will be removed and the credential section will be added back in.

One thing to think bout though is if we want to keep text parsing in to support older ddev cli versions, or if we want to enforce the next release of ddev cli that includes the json output to be the minimum version number?

Due to the fragile nature of text parsing, i'm much more of a fan of enforcing a minimum version going forward

@rfay
Copy link
Member

rfay commented Nov 17, 2017

Let's just support newer >=v0.9.4 ddev, IMO. You can do a ddev version -j and if it fails, say they need the latest. Or whatever.

I note that ddev version -j might need to be better for you. I don't like it much. It should give digestible info.

@andrew-c-tran
Copy link
Contributor

i've updated the PR to use the json output from the CLI and have updated the tests accordingly. I've also opened #54 to address the version checking need as it's a common need for both ddev-list and ddev-describe functionality.

@andrew-c-tran
Copy link
Contributor

merged #51

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants