-
Notifications
You must be signed in to change notification settings - Fork 394
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
WT-13560 Deprecate SSH Cloning of Git repositories #11068
Conversation
What makes this change safe?A good answer to this question helps the reviewers understand where they should focus their attention, so please consider these questions:
References: Checklist before requesting a review
|
@@ -544,7 +555,7 @@ functions: | |||
exit 0 | |||
fi | |||
|
|||
git clone git@github.com:wiredtiger/wiredtiger.github.com.git | |||
git clone https://github.com/wiredtiger/wiredtiger.github.com.git |
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.
wiredtiger/wiredtiger.github.com
is a public repo so https clone should work. The following shell.exec
command has already taken care of the token generation from github app (in a more manual way than the github.generate_token
Evergreen command).
@@ -128,6 +128,15 @@ functions: | |||
type: setup | |||
params: | |||
directory: wiredtiger | |||
"generate github token": &gen_github_token | |||
command: github.generate_token |
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.
The Github app has been provisioned in the "WiredTiger (develop)" Evergreen project to enable this command invocation.
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 👍
Some thoughts. No actions required.
@@ -21,7 +21,7 @@ We also suggest <a href="https://ninja-build.org/">Ninja</a> and | |||
First, clone the repository: | |||
|
|||
@code | |||
git clone git://github.com/wiredtiger/wiredtiger.git | |||
git clone https://github.com/wiredtiger/wiredtiger.git |
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.
Take this as a note and not an addressable comment. I believe https://
cloning was deprecated a while back, so developers that use https://
to clone on their workstations don't have permission to push branches to GitHub.
I don't think it's an issue for external contributors but I'm curious if this will impact new WT devs.
I have a new MacBook I'll set up in a week or so. I can investigate this then. Feel free to merge in the meantime
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.
Good point. I assume this public doc is mainly targeted at external contributors, who may run into issues with SSH clone (as they are not members of the repo). I double-checked our internal wiki and it does write SSH clone (which is good).
@@ -21,7 +21,7 @@ We also suggest <a href="https://ninja-build.org/">Ninja</a> and | |||
First, clone the repository: |
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.
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.
Good catch. It was a typo in the commit title.
@@ -5,7 +5,7 @@ last_stable_dir=wiredtiger_${last_stable}/ | |||
last_stable_branch=mongodb-${last_stable} | |||
|
|||
function setup_last_stable { | |||
git clone [email protected]:wiredtiger/wiredtiger.git ${last_stable_dir} |
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.
I don't see an evergreen config that calls this file. Do you know where it's executed from?
edit: The test talks about 4.2 and 4.4 and hasn't been touched in 4 years. Perhaps it was missed in the migration from Jenkins to Evergreen
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.
You could be right. Not sure if anyone uses this script locally. This change is optional with the motivation of aligning the git clone method with other places.
@@ -121,7 +121,7 @@ def prepare_branch(branch, config): | |||
print(f'Branch {branch} is already cloned') | |||
system(f'git -C "{path}" pull') | |||
else: | |||
source = '[email protected]:wiredtiger/wiredtiger.git' |
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.
I've added compatibility-test-suite
to the PR testing so we execute this new code
test/evergreen.yml
Outdated
@@ -136,8 +145,9 @@ functions: | |||
script: | | |||
set -o errexit | |||
set -o verbose | |||
echo "generated_token: ${generated_token}" |
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.
leftover?
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.
Good catch. Removed in ef6f023
Replace the existing SSH clones (i.e. git clone git@//github.com/<org>/<repo>) with either HTTP clones or the `github.generate_token` Evergreen project command, to support the deprecation of SSH cloning in Evergreen. Please note the change will need to be backported to all supported release branches. (cherry picked from commit f3cb304)
Replace the existing SSH clones (i.e. git clone git@//github.com//) with either HTTP clones or the
github.generate_token
Evergreen project command, to support the deprecation of SSH cloning in Evergreen.Please note the change will need to be backported to all supported release branches.