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

Add OpenSSH requirements to 02-install.md #9228

Merged
merged 2 commits into from
Sep 27, 2024
Merged

Conversation

codyfishman
Copy link
Contributor

Adding details about OpenSSH requirements for Terminus nested Drush and WP-CLI commands.

Verified this information in https://pantheon.slack.com/archives/C1P99MH3M/p1726675141658079.

Summary

Doc Page Title - <Enter a one sentence summation of the pertinent changes (including not-yet-completed work) provided by this PR.>

Adding details about OpenSSH requirements for Terminus nested Drush and WP-CLI commands.
@codyfishman codyfishman requested a review from a team as a code owner September 18, 2024 16:18
Copy link

⚡ Deployed with Pantheon Decoupled

This build was successfully deployed with Pantheon. You can track the build logs here.

👀 Preview: https://pr-9228-documentation.appa.pantheon.site
🛠️ Manage in Pantheon: https://dashboard.pantheon.io/site/2b30153f-e8b1-4427-b076-6109e704ba5d/overview

@@ -66,6 +66,9 @@ Terminus does not work with the following platforms:

### Recommended Packages

- [OpenSSH 7.8](https://github.com/PowerShell/Win32-OpenSSH/wiki/) or later
- You can check your OpenSSH version by running `ssh -V` from a terminal application.
- This is required package for executing nested Drush or WP-CLI commands.
Copy link
Member

@rachelwhitton rachelwhitton Sep 24, 2024

Choose a reason for hiding this comment

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

Since the copy says this package is required, it should be moved from "Recommended Packages" to the above section, "Required Packages". nvm it makes sense here after a second read

Should this also be added to the projects README?

@kporras07 @stovak can you review and weigh in?

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the idea of calling openssh out here (and maybe even moving it to required); I'm not sure I like linking it to PowerShell Win32-OpenSSH but I don't know of a better link. Would it make sense to NOT link anything here and allow customers to get the right package based on their OS?

Copy link
Contributor

Choose a reason for hiding this comment

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

100% agreement on also getting it to project's README. I can create a quick PR once we figure out how we want to do it here

Copy link
Member

Choose a reason for hiding this comment

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

@kporras07 I reviewed this again with @jazzsequence during docs coworking this morning, we decided to remove the link entirely (we don't link out to PHP in that same list so I think it's fine without), move OpenSSH up to the required packages section, and remove the recommended packages section. Can you do another review when convenient?

@rachelwhitton rachelwhitton self-assigned this Sep 24, 2024
@rachelwhitton rachelwhitton added Topic: Terminus Type: Quick Fix Issues that should be resolvable in a small PR labels Sep 24, 2024
Copy link

⚡ Deployed with Pantheon Decoupled

This build was successfully deployed with Pantheon. You can track the build logs here.

👀 Preview: https://pr-9228-documentation.appa.pantheon.site
🛠️ Manage in Pantheon: https://dashboard.pantheon.io/site/2b30153f-e8b1-4427-b076-6109e704ba5d/overview

Copy link
Contributor

@kporras07 kporras07 left a comment

Choose a reason for hiding this comment

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

LGTM

@rachelwhitton rachelwhitton merged commit d2573fe into main Sep 27, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Topic: Terminus Type: Quick Fix Issues that should be resolvable in a small PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants