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

CLI-1485: Add SSH URL in alias list #1842

Merged
merged 6 commits into from
Jan 24, 2025

Conversation

joshirohit100
Copy link
Contributor

@joshirohit100 joshirohit100 commented Jan 17, 2025

Adding SSH URL column for the drush alias list.

Impacted command - acli remote:aliases:list

Context - We can ssh on server using acli remote:drush ssh. I needed to download some file (basically scp) from the server to local but I didnt have the details of the ssh of server to do the scp as SCP.

So I think it will be helpfull if drush alias list also provide this details so that this can be used if required

Output before -

+-----------------------------------+-------------------+--------------------------------------------+
| Application                       | Environment Alias      |   Environment UUID                           |
+-----------------------------------+-------------------+--------------------------------------------+
| My Application                    |  myalias.dev           |   MYENVUUID |
+-----------------------------------+-------------------+--------------------------------------------+

Output after

+-----------------------------------+-------------------+----------------------------------
| Application    | Environment Alias | Environment UUID | SSH URL                                             |
+-----------------------------------+-------------------+--------------------------------------------+--
| My Application | myalias.dev       | MYENVUUID        | [email protected]  |

@joshirohit100 joshirohit100 changed the title Add / Show the SSH URL in alias list. Add SSH URL in alias list. Jan 17, 2025
Copy link

codecov bot commented Jan 17, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.92%. Comparing base (4d64180) to head (47867c9).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #1842   +/-   ##
=========================================
  Coverage     92.92%   92.92%           
  Complexity     1847     1847           
=========================================
  Files           123      123           
  Lines          6966     6968    +2     
=========================================
+ Hits           6473     6475    +2     
  Misses          493      493           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

Try the dev build for this PR: https://acquia-cli.s3.amazonaws.com/build/pr/1842/acli.phar

curl -OL https://acquia-cli.s3.amazonaws.com/build/pr/1842/acli.phar
chmod +x acli.phar

@danepowell
Copy link
Contributor

danepowell commented Jan 21, 2025

This makes the table about 160 characters wide, which is way beyond what I'd consider healthy (80-132 chars). I'm also not sure what's so special about the SSH URL that it should be displayed here: people could easily want other info such as the Git URL, domain names, etc...

I can think of a few solutions, let me know what you prefer:

  1. Move the application name out of the table since the command only takes a single application as an argument anyway. Put the name above the table to make room for the SSH URL.
  2. Add help text after the table like To get more information about a specific environment, run acli api:environments:find <alias>, so people know how to get additional info

@joshirohit100
Copy link
Contributor Author

Use case for SSH URL was -

  • I have access/subscription to an application in acquia cloud but don;t have application code on local
  • I can ssh to acquia cloud using acli drush
  • Then I needed to download some file to check/test from server and for that (scp command), I needed to know the SSH details (I could get that from acquia cloud but that required some steps)..

I think the missing part was as I didn;t know how to get the details after getting alias list.

So I think option-2 To get more information about a specific environment, run acli api:environments:find <alias> will be more helpful then

@danepowell
Copy link
Contributor

Sounds good, can you update the PR or open a new one to include that message?

@joshirohit100
Copy link
Contributor Author

Thanks! I have updated the MR.

Now this will look like -

+-----------------------------------+-------------------+-------------------------------
| Application    	   | Environment Alias   | Environment UUID                        |
+-----------------------------------+-------------------+--------------------------------
| Sample application 1 | devcloud2.dev       | 24-a47ac10b-58cc-4372-a567-0e02b2c3d470 |
+-----------------------------------+-------------------+-------------------------------

To get more information about a specific environment, run acli api:environments:find <alias>

@danepowell danepowell changed the title Add SSH URL in alias list. CLI-1485: Add SSH URL in alias list Jan 23, 2025
@danepowell
Copy link
Contributor

danepowell commented Jan 23, 2025

I think you're right that having the SSH URL in the table could be useful. It's a bit incongruous with the name of the command (maybe it should be renamed acli app:env-list or something), but still useful.

Let me know if you approve of my changes.

@joshirohit100
Copy link
Contributor Author

Yes, I think changing the command name make sense because as per the current command remote:aliases:list, app name is mandatory argument and command returns info about Application name, env alias and env uuid. And all of this info, application name looks redundant to me as this info is same for all the result + its already known.

Thus changing command name make sense to provide quick and useful info

@danepowell danepowell merged commit d69e55d into acquia:main Jan 24, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants