-
Notifications
You must be signed in to change notification settings - Fork 501
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?
Support self-hosted github installations #2755
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 <[email protected]>
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? |
mortenpi
left a comment
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.
src/deployconfig.jl
Outdated
| 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?
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 think we can assume anything about it, I don't know exactly if it is configurable but I would expect that it is as some orgs have strict rules about domain names
I need to check GITHUB_SERVER_URL , for some reason I haven't used that, don't know if it is because we don't have it or because I've missed it
if something like that exists I would prefer to used it indeed instead of specifying it manually
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.
Note that GITHUB_API_URL is mentioned in the GitHub manual as a variable they set in GitHub Action workflows.
And in the corresponding enterprise docs at https://docs.github.com/en/[email protected]/actions/reference/workflows-and-actions/variables the example value given is http(s)://HOSTNAME/api/v3 -- so no, we can't just assume it is api.$(host) (I assume that's the URL one gest if one enables "subdomain isolation")
|
@mortenpi I have number of issues in testing which I will address, but can you provide initial feedback about url parsing thing that I did based on your ideas, what do you think about it ? |
fingolfin
left a comment
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.
Thank you, I think this is quite useful and sensible. I still have a bunch of comments :-)
| Implementation of `DeployConfig` for deploying from GitHub Actions. | ||
| For self-hosted GitHub installation use `GitHubActions(host, pages_url)` constructor |
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.
| For self-hosted GitHub installation use `GitHubActions(host, pages_url)` constructor | |
| For self-hosted GitHub installation use the `GitHubActions(host, pages_url)` constructor |
| Implementation of `DeployConfig` for deploying from GitHub Actions. | ||
| For self-hosted GitHub installation use `GitHubActions(host, pages_url)` constructor | ||
| to specify the host name and a **full path** to the GitHub pages location. |
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 the "full path" referring to pages_url ? I find that confusing, a path is not an URL; maybe clarify the text?
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.
- review this description, seems that API update is not reflected here (is there constructor that takes
hostas argument?)
| 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.
Note that GITHUB_API_URL is mentioned in the GitHub manual as a variable they set in GitHub Action workflows.
And in the corresponding enterprise docs at https://docs.github.com/en/[email protected]/actions/reference/workflows-and-actions/variables the example value given is http(s)://HOSTNAME/api/v3 -- so no, we can't just assume it is api.$(host) (I assume that's the URL one gest if one enables "subdomain isolation")
| parts = split(github_repository, "/") | ||
| github_pages_url = if length(parts) == 2 | ||
| owner, repo = parts | ||
| "https://$(owner).github.io/$(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.
Of course you are just moving this around, and that's already a good step; so what I say below is not a change request, but a general thought (perhaps we should record it it into an issue)
Instead of hardcoding the pages URL like that (which won't if e.g. a custom domain name is used, as e.g. Documenter.jl does) one could also query the pages URL from the GitHub API. E.g. if the gh tool is installed:
$ gh api repos/JuliaDocs/Documenter.jl --jq '.homepage'
https://documenter.juliadocs.orgBut of course one also do that without, using just the github_api:
curl -s https://api.github.com/repos/JuliaDocs/Documenter.jl | jq -r '.homepage'
Obviously in Julia we'd use the JSON module, not jq, to parse this data.
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.
That would be great, however I can't find any public API that will tell me where github pages are located.
homepage is not reliable and can point somewhere else
There is private api /repos/{owner}/{repo}/pages but it requires authentication.
| # Regular tag build with GITHUB_TOKEN | ||
| withenv( | ||
| "GITHUB_EVENT_NAME" => "push", | ||
| "GITHUB_EVENT_NAME" => "push", "GITHUB_SERVER_URL" => "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.
Let's keep this to one per line?
| "GITHUB_EVENT_NAME" => "push", "GITHUB_SERVER_URL" => "github.com", | |
| "GITHUB_EVENT_NAME" => "push", | |
| "GITHUB_SERVER_URL" => "github.com", |
and then the same everywhere below.
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.
- check that this is not because of auto format and do the change uniformly
| logger = SimpleLogger(buffer, Logging.Debug) | ||
| with_logger(logger) do | ||
| withenv( | ||
| "GITHUB_EVENT_NAME" => "push", "GITHUB_SERVER_URL" => "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.
| "GITHUB_EVENT_NAME" => "push", "GITHUB_SERVER_URL" => "github.com", | |
| "GITHUB_EVENT_NAME" => "push", | |
| "GITHUB_SERVER_URL" => "github.com", |
Co-authored-by: Max Horn <[email protected]>

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!