-
Notifications
You must be signed in to change notification settings - Fork 498
Support self-hosted github installations #2755
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
base: master
Are you sure you want to change the base?
Conversation
60c5931
to
f3853c9
Compare
f3853c9
to
ef38eb7
Compare
This looks fine to me! Could we also make sure that it's documented in the relevant docstrings please?
|
Co-authored-by: Morten Piibeleht <morten.piibeleht@gmail.com>
I've updated documentation and hopefully - coverage.
The one above will probably work, but the one bellow will not (not because of my changes)
Is it just me reading documentation in a wrong way? |
2964c67
to
438901a
Compare
Do we have somewhere where we advertise passing e.g. |
@mortenpi , can you review this please? |
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.
Sorry for the delay. Unfortunately, I do have a couple of questions. I think my main concern is to make sure this doesn't affect existing setups.
""" | ||
GitHub(user :: AbstractString, repo :: AbstractString) | ||
GitHub(user :: AbstractString, repo :: AbstractString, [host :: AbstractString]) | ||
GitHub(remote :: AbstractString) |
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 took the liberty of removing host
for the single-argument method. I think it would be better if it stays single-argument. If we want to add support for overriding the host
, then let's figure out some syntax for the string, like github.selfhosted:Org/Repo.jl
, which we can regex.
github_api = get(ENV, "GITHUB_API_URL", "") # https://api.github.com | ||
|
||
# Compute GitHub Pages URL from repository | ||
parts = split(github_repository, "/") | ||
github_pages_url = if length(parts) == 2 | ||
owner, repo = parts | ||
"https://$(owner).github.io/$(repo)/" | ||
else | ||
"" | ||
end | ||
|
||
return GitHubActions(github_repository, github_event_name, github_ref, "github.com", github_api, github_pages_url) |
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.
If we hard-code github_host
here, it feels like we should also hard-code the API URL? I also have a minor security concern here, where this could allow someone redirect the API calls somewhere (including the token) by somehow attacking the GITHUB_API_URL
environment variable (for normal GitHub-hosted repos).
github_api = get(ENV, "GITHUB_API_URL", "") # https://api.github.com | |
# Compute GitHub Pages URL from repository | |
parts = split(github_repository, "/") | |
github_pages_url = if length(parts) == 2 | |
owner, repo = parts | |
"https://$(owner).github.io/$(repo)/" | |
else | |
"" | |
end | |
return GitHubActions(github_repository, github_event_name, github_ref, "github.com", github_api, github_pages_url) | |
# Compute GitHub Pages URL from repository | |
parts = split(github_repository, "/") | |
github_pages_url = if length(parts) == 2 | |
owner, repo = parts | |
"https://$(owner).github.io/$(repo)/" | |
else | |
"" | |
end | |
return GitHubActions(github_repository, github_event_name, github_ref, "github.com", "https://api.github.com", github_pages_url) |
github_repository = get(ENV, "GITHUB_REPOSITORY", "") # "JuliaDocs/Documenter.jl" | ||
github_event_name = get(ENV, "GITHUB_EVENT_NAME", "") # "push", "pull_request" or "cron" (?) | ||
github_ref = get(ENV, "GITHUB_REF", "") # "refs/heads/$(branchname)" for branch, "refs/tags/$(tagname)" for tags | ||
github_api = get(ENV, "GITHUB_API_URL", "") # https://api.github.com |
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.
Is GITHUB_API_URL
something that is configurable for a self-hosted instance, or can we assume it's api.$(host)
?
There's also GITHUB_SERVER_URL
, which could potentially be used for determining host
automatically? Or would that not be reliable and/or two automagical?
try | ||
Sys.which("curl") === nothing && return | ||
## Extract owner and repository name | ||
m = match(r"^github.com\/(.+?)\/(.+?)(.git)?$", deploydocs_repo) | ||
m = match(Regex("^(?:https?://)?$(cfg.github_host)\\/(.+?)\\/(.+?)(.git)?\$"), deploydocs_repo) |
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 feel like that for clarity, it would be better to capture the host and string-compare later, rather than dynamically constructing the regex.
This PR enables support for self-hosted GitHub installations
I've tested it with the following setup
Both deployment and notification worked well
Was made during JuliaCon 2025 hackathon!