-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
env :: support ssh protocol for github repos. #40451
Conversation
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @n3f! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
27cec8c
to
20cfaaf
Compare
20cfaaf
to
c6ad9cc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This tests well for me!
I did notice one little weird thing, but it doesn't cause problems. I think the HTTPS clone is cloning directly to $wp_env_dir/wordpress
, but the SSH clone went to $wp_env_dir/wordpress/wordpress
. However, docker-compose still maps it correctly ($wp_env_path/WordPress/WordPress:/var/www/html
), so it doesn't cause problems.
Also, would you be able to update the Changelog file in packages/env with this new feature? |
c6ad9cc
to
b54e4de
Compare
I'm not sure about my changelog entry 😆, I put today's date but not sure if that should be different or if everything else looks okay. I'm not sure how merging occurs, since I don't have the permissions, I assume when it all looks good, you'll just push the button? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM and still tests well. I think we need to tweak the changelog a bit, but after that I can hit the merge button. (I don't think I have write access to the branch, otherwise I'd update it myself and merge now!)
Apply suggestion Co-authored-by: Noah Allen <noahtallen@gmail.com>
Accepted the suggestion (and added you as a contributor to my fork if anything else needs to be updated). |
Great, thanks! I'll merge once the tests finish :) |
Add support for ssh strings in the @wordpress/env package sources. (A similar issue was discussed in #20325).
What?
Adds URL parsing for ssh strings in
env
parse-config
module (which returns aWP_Source
object).Why?
Git parsing only supported HTTP (until this PR), but fails to pull/clone authenticated projects.
How?
The git library already supports ssh; this change just implements parsing into a supported
WP_Source
.Testing Instructions
wp-env
enabled project. (e.g.ssh://git@github.com/some/plugin.git
)<gutenberg-dir>/packages/env/bin/wp-env start