LibGit2 callback error in automerge for private registry #245
Comments
|
Just to see, I tried running the integration tests with my test repo set to private, and they all passed. I got warnings like though. To me, the issue looks like LibGit2 not picking up the credentials properly when trying to clone in this line: RegistryCI.jl/src/AutoMerge/util.jl Line 25 in 4f1acbd Not sure why that is, but also it seems like the registry gets cloned anyway by , so maybe we don't need to clone it again in ? |
|
We also need the latest master of the registry. We can't get that from I guess we need a way to pass credentials when doing clones. |
|
Maybe something like would work? Not super safe, since someone could theoretically submit a PR that adds a |
|
@omus may have thoughts or insight regarding git credential helpers that could be useful here. |
Can't we though? Seems like this is doable just reading https://github.com/actions/checkout, but I'm new to GitHub Actions in general so I could be misunderstanding. Seems like diff --git a/example_github_workflow_files/automerge.yml b/example_github_workflow_files/automerge.yml
index aaf42fd..cbf8559 100644
--- a/example_github_workflow_files/automerge.yml
+++ b/example_github_workflow_files/automerge.yml
@@ -15,6 +15,9 @@ jobs:
os: [ubuntu-latest]
steps:
- uses: actions/checkout@af513c7a016048ae468971c52ed77d9562c7c819 # v1.0.0
+ - uses: actions/checkout@v2
+ with:
+ ref: master
+ path: registry-repo-master
- uses: julia-actions/setup-julia@082493e5c5d32c1fa68c35556429b0f1b2807453 # v1.0.1
with:
version: ${{ matrix.julia-version }}
diff --git a/src/AutoMerge/pull-requests.jl b/src/AutoMerge/pull-requests.jl
index 86d987a..e82efa8 100644
--- a/src/AutoMerge/pull-requests.jl
+++ b/src/AutoMerge/pull-requests.jl
@@ -54,6 +54,9 @@ function pull_request_build(pr_number::Integer,
return result
end
+# defined in `automerge.yml`
+const REGISTRY_REPO_MASTER_WORKSPACE_LOCATION = "registry-repo-master"
+
function pull_request_build(pr::GitHub.PullRequest,
current_pr_head_commit_sha::String,
registry::GitHub.Repo,
@@ -67,42 +70,19 @@ function pull_request_build(pr::GitHub.PullRequest,
whoami::String,
registry_deps::Vector{<:AbstractString} = String[])::Nothing
if is_new_package(pr)
- registry_master = clone_repo(registry)
- if !master_branch_is_default_branch
- checkout_branch(registry_master, master_branch)
- end
- pull_request_build(NewPackage(),
- pr,
- current_pr_head_commit_sha,
- registry;
- auth = auth,
- authorized_authors=authorized_authors,
- authorized_authors_special_jll_exceptions=authorized_authors_special_jll_exceptions,
- registry_head = registry_head,
- registry_master = registry_master,
- suggest_onepointzero = suggest_onepointzero,
- whoami=whoami,
- registry_deps = registry_deps)
- rm(registry_master; force = true, recursive = true)
- elseif is_new_version(pr)
- registry_master = clone_repo(registry)
- if !master_branch_is_default_branch
- checkout_branch(registry_master, master_branch)
- end
- pull_request_build(NewVersion(),
- pr,
- current_pr_head_commit_sha,
- registry;
- auth = auth,
- authorized_authors=authorized_authors,
- authorized_authors_special_jll_exceptions=authorized_authors_special_jll_exceptions,
- registry_head = registry_head,
- registry_master = registry_master,
- suggest_onepointzero = suggest_onepointzero,
- whoami=whoami,
- registry_deps = registry_deps)
- rm(registry_master; force = true, recursive = true)
+ kind = NewPackage()
+ elseif is_new_version(pr)
+ kind = NewVersion()
else
throw(AutoMergeNeitherNewPackageNorNewVersion("Neither a new package nor a new version. Exiting..."))
end
+ pull_request_build(kind, pr, current_pr_head_commit_sha, registry;
+ auth=auth,
+ authorized_authors=authorized_authors,
+ authorized_authors_special_jll_exceptions=authorized_authors_special_jll_exceptions,
+ registry_head=registry_head,
+ registry_master=REGISTRY_REPO_MASTER_WORKSPACE_LOCATION,
+ suggest_onepointzero=suggest_onepointzero,
+ whoami=whoami,
+ registry_deps=registry_deps)
end
diff --git a/src/AutoMerge/util.jl b/src/AutoMerge/util.jl
index e4fb89f..468aadd 100644
--- a/src/AutoMerge/util.jl
+++ b/src/AutoMerge/util.jl
@@ -7,25 +7,6 @@ function checkout_branch(dir::AbstractString,
cd(original_working_directory)
end
-clone_repo(repo::GitHub.Repo) = clone_repo(repo_url(repo))
-
-function clone_repo(url::AbstractString)
- parent_dir = mktempdir()
- atexit(() -> rm(parent_dir; force = true, recursive = true))
- repo_dir = joinpath(parent_dir, "REPO")
- my_retry(() -> _clone_repo_into_dir(url, repo_dir))
- @info("Clone was successful")
- return repo_dir
-end
-
-function _clone_repo_into_dir(url::AbstractString, repo_dir)
- @info("Attempting to clone...")
- rm(repo_dir; force = true, recursive = true)
- mkpath(repo_dir)
- LibGit2.clone(url, repo_dir)
- return repo_dir
-end
-
function _comment_disclaimer()
result = string("\n\n",
"Note that the guidelines are only required for the pull request ",I can make a PR with this patch if this is the right approach. |
|
In the past, I have run into the issue where The only way I was able to fix it is by manually cloning the latest master myself. The solution here is for us to remove the use of LibGit2 and instead call command-line git. Then e.g. you can add an ssh key with the appropriate read permissions, and add it to ssh agent, and then when I use command-line git to clone, it will work. |
|
Is there a reference to that Regardless, if |
|
Just dogfooded the patch above over at Beacon's private registry; it does seem to resolve the issue in the OP + clone master correctly (though maybe I just got lucky, if it's an intermittent issue that Dilum was referring to), but there's still a remaining issue of propagating read credentials to To resolve that, we're applying the workflow step suggested by @ararslan above which configures the |
|
So, in the end, the above worked, but we had to make the following additional patch in our fork of RegistryCI or else we still got diff --git a/src/AutoMerge/guidelines.jl b/src/AutoMerge/guidelines.jl
index 03e3fee..885d306 100644
--- a/src/AutoMerge/guidelines.jl
+++ b/src/AutoMerge/guidelines.jl
@@ -310,31 +310,14 @@ function _run_pkg_commands(working_directory::String,
failure_message,
failure_return_1,
failure_return_2)
- original_directory = pwd()
- tmp_dir_1 = mktempdir()
- tmp_dir_2 = mktempdir()
- atexit(() -> rm(tmp_dir_1; force = true, recursive = true))
- atexit(() -> rm(tmp_dir_2; force = true, recursive = true))
- cd(tmp_dir_1)
- # We need to be careful with what environment variables we pass to the child
- # process. For example, we don't want to pass an environment variable containing
- # our GitHub token to the child process. Because if the Julia package that we are
- # testing has malicious code in its __init__() function, it could try to steal
- # our token. So we only pass four environment variables:
- # 1. PATH. If we don't pass PATH, things break. And PATH should not contain any
- # sensitive information.
- # 2. PYTHON. We set PYTHON to the empty string. This forces any packages that use
- # PyCall to install their own version of Python instead of using the system
- # Python.
- # 3. JULIA_DEPOT_PATH. We set JULIA_DEPOT_PATH to the temporary directory that
- # we created. This is because we don't want the child process using our
- # real Julia depot. So we set up a fake depot for the child process to use.
- # 4. R_HOME. We set R_HOME to "*".
- cmd = Cmd(`$(Base.julia_cmd()) -e $(code)`;
- env = Dict("PATH" => ENV["PATH"],
- "PYTHON" => "",
- "JULIA_DEPOT_PATH" => tmp_dir_2,
- "R_HOME" => "*"))
+ # XXX: upstream RegistryCI.jl passes `ENV` here with only a few restricted
+ # variables to disallow the to-be-registered packages from escalating
+ # priveleges/stealing tokens from within their `__init__` function and
+ # enforcing better e.g. Python dependency management. However, this also
+ # breaks git credential setup for private registries. Since Beacon's registry
+ # isn't publically available anyway, this isn't really a problem for us, so
+ # we simply patched it out to allow us to use this privately....
+ cmd = Cmd(`$(Base.julia_cmd()) -e $(code)`)
# GUI toolkits may need a display just to load the package
xvfb = Sys.which("xvfb-run")
@info("xvfb: ", xvfb)
@@ -344,9 +327,6 @@ function _run_pkg_commands(working_directory::String,
end
@info(before_message)
cmd_ran_successfully = success(pipeline(cmd, stdout=stdout, stderr=stderr))
- cd(original_directory)
- rm(tmp_dir_1; force = true, recursive = true)
- rm(tmp_dir_2; force = true, recursive = true)
if cmd_ran_successfully
@info(success_message)
return success_return_1, success_return_2This obviously isn't a patch that could be applied to JuliaRegistries/RegistryCI (since it has to run on public non-vetted submissions), but out of curiosity, does anybody have an idea for why stripping the |
|
I can see how reducing the set environmental variables is causing non-interactive cloning to fail as the git credential helper system calls out to the shell to run helpers (this is just how the interface works). I think I can assist in debugging this with a little more information. Can you confirm you are attempting to clone via HTTPS? Running the following should show any credential helper details which could/should be used: git config --show-origin --get-regexp 'credential.*' |
|
Turns out this is just Git not knowing how to apply its global config ( |
306: Pass HOME to subprocesses r=christopher-dG a=christopher-dG Closes #245 Lots of stuff needs HOME to work properly. More concretely, both Git CLI and LibGit2 get their config from `$HOME/.gitconfig` by default, and that breaks when it's unset. Co-authored-by: Chris de Graaf <me@cdg.dev>
306: Pass HOME to subprocesses r=christopher-dG a=christopher-dG Closes #245 Lots of stuff needs HOME to work properly. More concretely, both Git CLI and LibGit2 get their config from `$HOME/.gitconfig` by default, and that breaks when it's unset. Co-authored-by: Chris de Graaf <me@cdg.dev>
With an automerge.yaml set up nearly identically to that used in General, we've been getting the following consistently on our private registry:
The text was updated successfully, but these errors were encountered: