Skip to content

add support for cloning Git repository via SSH rather than HTTPS #300

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

Merged
merged 36 commits into from
Jun 23, 2025
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
e96ec76
Merge pull request #228 from EESSI/develop
boegel Nov 14, 2023
66ba1f9
Merge pull request #234 from EESSI/develop
boegel Nov 26, 2023
6ba9625
Merge pull request #240 from EESSI/develop
boegel Jan 30, 2024
fa91fcd
Merge pull request #259 from EESSI/develop
laraPPr Feb 28, 2024
a5b40e9
Merge pull request #270 from EESSI/develop
Neves-P May 15, 2024
357f9df
Merge pull request #285 from EESSI/develop
boegel Sep 18, 2024
a444bcc
add support for cloning Git repository via SSH rather than HTTPS
boegel Feb 6, 2025
6dcdd80
fix use of config.BUILDENV_SETTING_CLONE_GIT_REPO_VIA
boegel Feb 6, 2025
3f2cfcb
also use clone_git_repo_via in app.cfg.example
boegel Feb 6, 2025
37c06af
mark clone_git_repo_via as optional + add logging
boegel Feb 6, 2025
4b046f6
fix get_build_env_cfg to get clone_git_repo_via value from buildenv i…
boegel Feb 6, 2025
45bd556
remove curl command to obtain PR diff + fix _ERROR_GIT_CLONE constant
boegel Feb 6, 2025
b19fe23
fix return value for 'git diff' command in download_pr function
boegel Feb 6, 2025
5c51665
make sure that download_comment is defined in comment_download_pr
boegel Feb 6, 2025
a1c2635
fix 'git diff' command in download_repo
boegel Feb 6, 2025
02f3125
fix branch name used by 'git diff' command
boegel Feb 6, 2025
47e2076
Merge branch 'main' into git_clone_via_ssh
boegel Feb 6, 2025
c7a98f2
comment out clone_git_repo_via setting in REQUIRED_CONFIG to avoid ma…
boegel Feb 21, 2025
7da317d
fix order in REQUIRED_CONFIG (recommended settings before optional ones)
boegel Feb 21, 2025
293ce3c
add more info on use case of clone_git_repo_via set to 'ssh'
boegel Feb 21, 2025
668915c
also add more info on use case of clone_git_repo_via set to 'ssh' to …
boegel Feb 21, 2025
882354c
Merge branch 'git_clone_via_ssh' of github.com:boegel/eessi-bot-softw…
boegel Feb 21, 2025
a99ae60
only use 'git diff' to obtain PR diff when 'ssh' is used to clone rep…
boegel Feb 21, 2025
4d9dc0e
Merge pull request #306 from EESSI/develop
boegel Mar 13, 2025
9b582ab
git diff from merge-base instead of from HEAD
smoors Apr 4, 2025
1ea33d9
Merge pull request #1 from smoors/pr300
boegel Apr 25, 2025
df1a258
Merge branch 'develop' into git_clone_via_ssh
boegel Apr 25, 2025
46d14a9
Merge branch 'git_clone_via_ssh' of github.com:boegel/eessi-bot-softw…
boegel Apr 25, 2025
f4c0940
update README
smoors May 22, 2025
95c574b
Merge pull request #319 from EESSI/develop
bedroge May 23, 2025
4021c20
Merge pull request #2 from smoors/pr300
boegel May 28, 2025
4a3dd94
Merge branch 'develop' into git_clone_via_ssh
boegel May 28, 2025
e30cd5b
Merge branch 'git_clone_via_ssh' of github.com:boegel/eessi-bot-softw…
boegel May 28, 2025
a985803
Merge branch 'main' into git_clone_via_ssh
boegel Jun 19, 2025
1381689
Merge branch 'develop' into git_clone_via_ssh
boegel Jun 20, 2025
750b8a4
add pr_diff_failure and pr_diff_tip to required configuration settings
boegel Jun 20, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -443,6 +443,15 @@ variables) that are allowed to be specified in a PR command with the
be exported into the build environment before running the bot/build.sh script.


```
clone_git_repo_via = https
```

The `clone_git_repo_via` setting specifies via which mechanism the Git repository
should be cloned. This can be either:
* `https` (default): clone repository via HTTPS with `git clone https://github.com/<owner>/<repo>`
* `ssh`: clone repository via SSH with `git clone git@github.com:<owner>/<repo>.git`

#### `[bot_control]` section

The `[bot_control]` section contains settings for configuring the feature to
Expand Down
4 changes: 4 additions & 0 deletions app.cfg.example
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,10 @@ allow_update_submit_opts = false
# exported into the build environment via `exportvariable` filters
allowed_exportvars = ["NAME1=value_1a", "NAME1=value_1b", "NAME2=value_2"]

# mechanisn to use to clone Git repository
# 'https' to clone via HTTPS (git clone https://github.com/<owner>/<repo>)
# 'ssh' to clone via SSH (git clone git@github.com:<owner>/<repo>.git)
clone_git_repo_via = https

[deploycfg]
# script for uploading built software packages
Expand Down
1 change: 1 addition & 0 deletions eessi_bot_event_handler.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
config.BUILDENV_SETTING_BUILD_JOB_SCRIPT, # required
config.BUILDENV_SETTING_BUILD_LOGS_DIR, # optional+recommended
config.BUILDENV_SETTING_BUILD_PERMISSION, # optional+recommended
# config.BUILDENV_SETTING_CLONE_GIT_REPO_VIA, # optional
config.BUILDENV_SETTING_CONTAINER_CACHEDIR, # optional+recommended
# config.BUILDENV_SETTING_CVMFS_CUSTOMIZATIONS, # optional
# config.BUILDENV_SETTING_HTTPS_PROXY, # optional
Expand Down
48 changes: 33 additions & 15 deletions tasks/build.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,8 @@
_ERROR_CURL = "curl"
_ERROR_GIT_APPLY = "git apply"
_ERROR_GIT_CHECKOUT = "git checkout"
_ERROR_GIT_CLONE = "curl"
_ERROR_GIT_CLONE = "git clone"
_ERROR_GIT_DIFF = "git diff"
_ERROR_NONE = "none"

# other constants
Expand Down Expand Up @@ -147,6 +148,10 @@ def get_build_env_cfg(cfg):
log(f"{fn}(): load_modules '{load_modules}'")
config_data[config.BUILDENV_SETTING_LOAD_MODULES] = load_modules

clone_git_repo_via = buildenv.get(config.BUILDENV_SETTING_CLONE_GIT_REPO_VIA, None)
log(f"{fn}(): clone_git_repo_via '{clone_git_repo_via}'")
config_data[config.BUILDENV_SETTING_CLONE_GIT_REPO_VIA] = clone_git_repo_via

return config_data


Expand Down Expand Up @@ -355,7 +360,7 @@ def clone_git_repo(repo, path):
return (clone_output, clone_error, clone_exit_code)


def download_pr(repo_name, branch_name, pr, arch_job_dir):
def download_pr(repo_name, branch_name, pr, arch_job_dir, clone_via=None):
"""
Download pull request to job working directory

Expand All @@ -364,6 +369,7 @@ def download_pr(repo_name, branch_name, pr, arch_job_dir):
branch_name (string): name of the base branch of the pull request
pr (github.PullRequest.PullRequest): instance representing the pull request
arch_job_dir (string): working directory of the job to be submitted
clone_via (string): mechanism to clone Git repository, should be 'https' (default) or 'ssh'

Returns:
None (implicitly), in case an error is caught in the git clone, git checkout, curl,
Expand All @@ -376,7 +382,19 @@ def download_pr(repo_name, branch_name, pr, arch_job_dir):
# - 'git checkout' base branch of pull request
# - 'curl' diff for pull request
# - 'git apply' diff file
clone_output, clone_error, clone_exit_code = clone_git_repo(f'https://github.com/{repo_name}', arch_job_dir)
log(f"Cloning Git repo via: {clone_via}")
if clone_via in (None, 'https'):
repo_url = f'https://github.com/{repo_name}'
elif clone_via == 'ssh':
repo_url = f'git@github.com:{repo_name}.git'
else:
clone_output = ''
clone_error = f"Unknown mechanism to clone Git repo: {clone_via}"
clone_exit_code = 1
error_stage = _ERROR_GIT_CLONE
return clone_output, clone_error, clone_exit_code, error_stage

clone_output, clone_error, clone_exit_code = clone_git_repo(repo_url, arch_job_dir)
if clone_exit_code != 0:
error_stage = _ERROR_GIT_CLONE
return clone_output, clone_error, clone_exit_code, error_stage
Expand All @@ -393,19 +411,16 @@ def download_pr(repo_name, branch_name, pr, arch_job_dir):
error_stage = _ERROR_GIT_CHECKOUT
return checkout_output, checkout_err, checkout_exit_code, error_stage

curl_cmd = ' '.join([
'curl -L',
'-H "Accept: application/vnd.github.diff"',
'-H "X-GitHub-Api-Version: 2022-11-28"',
f'https://api.github.com/repos/{repo_name}/pulls/{pr.number} > {pr.number}.diff',
git_diff_cmd = ' && '.join([
f"git fetch origin pull/{pr.number}/head:pr{pr.number}",
f"git diff HEAD pr{pr.number} > {pr.number}.diff",
Copy link
Contributor

@smoors smoors Feb 6, 2025

Choose a reason for hiding this comment

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

if you're now creating the diff like this, shouldn't you remove the curl_cmd above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, of course, I'll tackle that too

Copy link
Contributor

@smoors smoors Feb 6, 2025

Choose a reason for hiding this comment

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

edit: nvm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@smoors made a good point (in Slack) about this change: this doesn't take into account that there may be changes in the main branch that are not included in the PR branch yet.

So, a better approach would be to:

  • fetch the PR branch
  • check out the PR branch
  • git merge main
  • swap back to main branch
  • git diff
  • git apply

Copy link
Contributor

Choose a reason for hiding this comment

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

This procedure hasn't been implemented yet, right?

A few thoughts:

  • main should probably be the target branch of the PR, for example, for EESSI/2023.06 that would be 2023.06-software.eessi.io?
  • the git diff after the swap back to "main" is really just the plain git diff or rather git diff PR_branch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to determine base branch for PR (the target branch), maybe git merge-base is useful...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll change this so the new approach based on git is only used when clone_via is set to 'ssh' (so nothing changes for existing bots that listen to a public repo).

That gives us some freedom to experiment with the situation where the target branch has been updated since the branch for the PR was created.

Copy link
Contributor Author

@boegel boegel Feb 21, 2025

Choose a reason for hiding this comment

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

I've updated this in a99ae60 to only use the git diff approach when clone_git_repo_via is used.

It's still doing only git fetch + git diff, so you could still have issues when the PR branch is way behind on the target branch, I'll try to look into that next.

Copy link
Contributor

Choose a reason for hiding this comment

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

created a PR to this PR using git merge-base:
boegel#1

])
log(f'curl with command {curl_cmd}')
curl_output, curl_error, curl_exit_code = run_cmd(
curl_cmd, "Obtain patch", arch_job_dir, raise_on_error=False
git_diff_output, git_diff_error, git_diff_exit_code = run_cmd(
git_diff_cmd, "Obtain patch", arch_job_dir, raise_on_error=False
)
if curl_exit_code != 0:
error_stage = _ERROR_CURL
return curl_output, curl_error, curl_exit_code, error_stage
if git_diff_exit_code != 0:
error_stage = _ERROR_GIT_DIFF
return git_diff_output, git_diff_error, git_diff_exit_code, error_stage

git_apply_cmd = f'git apply {pr.number}.diff'
log(f'git apply with command {git_apply_cmd}')
Expand Down Expand Up @@ -457,6 +472,8 @@ def comment_download_pr(base_repo_name, pr, download_pr_exit_code, download_pr_e
download_comment = (f"```{download_pr_error}```\n"
f"{download_pr_comments_cfg[config.DOWNLOAD_PR_COMMENTS_SETTING_GIT_APPLY_FAILURE]}"
f"\n{download_pr_comments_cfg[config.DOWNLOAD_PR_COMMENTS_SETTING_GIT_APPLY_TIP]}")
else:
download_comment = f"```{download_pr_error}```"

download_comment = pr_comments.create_comment(
repo_name=base_repo_name, pr_number=pr.number, comment=download_comment
Expand Down Expand Up @@ -615,8 +632,9 @@ def prepare_jobs(pr, cfg, event_info, action_filter):
log(f"{fn}(): job_dir '{job_dir}'")

# TODO optimisation? download once, copy and cleanup initial copy?
clone_git_repo_via = build_env_cfg.get(config.BUILDENV_SETTING_CLONE_GIT_REPO_VIA)
download_pr_output, download_pr_error, download_pr_exit_code, error_stage = download_pr(
base_repo_name, base_branch_name, pr, job_dir
base_repo_name, base_branch_name, pr, job_dir, clone_via=clone_git_repo_via,
)
comment_download_pr(base_repo_name, pr, download_pr_exit_code, download_pr_error, error_stage)
# prepare job configuration file 'job.cfg' in directory <job_dir>/cfg
Expand Down
1 change: 1 addition & 0 deletions tools/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
BUILDENV_SETTING_BUILD_JOB_SCRIPT = 'build_job_script'
BUILDENV_SETTING_BUILD_LOGS_DIR = 'build_logs_dir'
BUILDENV_SETTING_BUILD_PERMISSION = 'build_permission'
BUILDENV_SETTING_CLONE_GIT_REPO_VIA = 'clone_git_repo_via'
BUILDENV_SETTING_CONTAINER_CACHEDIR = 'container_cachedir'
BUILDENV_SETTING_CVMFS_CUSTOMIZATIONS = 'cvmfs_customizations'
BUILDENV_SETTING_HTTPS_PROXY = 'https_proxy'
Expand Down
Loading