jekyll / github-metadata

Jekyll plugin to propagate the `site.github` namespace and set default values for use with GitHub Pages.
https://jekyll.github.io/github-metadata/
MIT License
282 stars 127 forks source link

Does not respect default branch of fork #249

Closed timwis closed 1 year ago

timwis commented 1 year ago

Hi team, we make use of this plugin in JKAN, a Jekyll-powered open data catalog. I've noticed that this plugin seems to use the default branch of the upstream repository, rather than the fork.

The only property made available is site.github.source.branch, which refers to upstream. In addition, github_edit_link uses the upstream branch in the URL it generates.

The issue with this—apart from it being different than what I assume people would expect—is that if someone forks JKAN and, say, switches to the new GitHub Actions based workflow for publishing GitHub Pages, then updates their default branch to main since they're not using the gh-pages workflow anymore, the links on the page will stop working. Another instance might be a case where an upstream repo uses master as the default branch, and the fork changes it to main.

I'm a bit surprised this hasn't come up before—to the point that I'm wondering if I'm missing something. Is there a reason it's like this?

Looking at the GitHub API, the source property does appear to refer to the upstream repo in the case of a fork, so I propose that rather than changing that property, we simply add the property default_branch from the api. I'd be happy to submit a PR to that effect, but wanted to check if it's welcome first.

Thanks all.

jekyllbot commented 1 year ago

This issue has been automatically marked as stale because it has not been commented on for at least two months.

The resources of the Jekyll team are limited, and so we are asking for your help.

If this is a bug and you can still reproduce this error on the master/main branch, please reply with all of the information you have about it in order to keep the issue open.

If this is a feature request, please consider whether it can be accomplished in another way. If it cannot, please elaborate on why it is core to this project and why you feel more than 80% of users would find this beneficial.

This issue will automatically be closed in two months if no further activity occurs. Thank you for all your contributions.

timwis commented 1 year ago

Hi there jekyllbot, nice to see you again. Just to confirm that yes, this is still an issue/bug.

parkr commented 1 year ago

Looking at the GitHub API, the source property does appear to refer to the upstream repo in the case of a fork

@yoannchaudet Hi! Is this intended behavior of the source result from the GitHub API? For a forked repo, source appears to read from the upstream, rather than the forked repo. I think we use the mister-fantastic preview, but I can't remember the details of how that preview changes the API.

so I propose that rather than changing that property, we simply add the property default_branch from the api. I'd be happy to submit a PR to that effect, but wanted to check if it's welcome first.

Sounds reasonable. Feel free to ship a PR.

timwis commented 1 year ago

Hey folks, upon further investigation, I think there may actually be a bug with the GitHub API that's causing this.

repository.rb appears to call the pages resource of the github api. I tried running that manually:

curl -L \
  -H "Accept: application/vnd.github+json" \
  -H "Authorization: Bearer <YOUR-TOKEN>"\
  -H "X-GitHub-Api-Version: 2022-11-28" \
  https://api.github.com/repos/timwis/jkan/pages

and it returns:

{
  "url": "https://api.github.com/repos/timwis/jkan/pages",
  "status": "built",
  "cname": null,
  "custom_404": false,
  "html_url": "http://timwis.com/jkan/",
  "build_type": "workflow",
  "source": {
    "branch": "gh-pages",
    "path": "/"
  },
  "public": true,
  "protected_domain_state": null,
  "pending_domain_unverified_at": null,
  "https_certificate": {
    "state": "approved",
    "description": "The certificate has been approved.",
    "domains": [
      null,
      "www.timwis.com"
    ],
    "expires_at": "2023-07-05"
  },
  "https_enforced": false
}

But the branch gh-pages no longer exists on timwis/jkan.

Similarly, running the request on azavea/opendataphilly-jkan returns that the source.branch is v2, which also no longer exists on that repo.

(Note that I've tried swapping the Accept header with application/vnd.github.mister-fantastic-preview+json, and running it on timwis-test/jkan, which is a fork and does not use a custom domain)

I suspect this may have something to do with the fact that we're publishing to GitHub Pages via an Actions workflow rather than the traditional branch auto-publish, and perhaps the Pages resource of the API hasn't been updated when that feature was released?

jekyllbot commented 1 year ago

This issue has been automatically marked as stale because it has not been commented on for at least two months.

The resources of the Jekyll team are limited, and so we are asking for your help.

If this is a bug and you can still reproduce this error on the master/main branch, please reply with all of the information you have about it in order to keep the issue open.

If this is a feature request, please consider whether it can be accomplished in another way. If it cannot, please elaborate on why it is core to this project and why you feel more than 80% of users would find this beneficial.

This issue will automatically be closed in two months if no further activity occurs. Thank you for all your contributions.

parkr commented 1 year ago

Were you able to get an answer to your API question above? Sounds like a good question to ask Support.

jekyllbot commented 1 year ago

This issue has been automatically marked as stale because it has not been commented on for at least two months.

The resources of the Jekyll team are limited, and so we are asking for your help.

If this is a bug and you can still reproduce this error on the master/main branch, please reply with all of the information you have about it in order to keep the issue open.

If this is a feature request, please consider whether it can be accomplished in another way. If it cannot, please elaborate on why it is core to this project and why you feel more than 80% of users would find this beneficial.

This issue will automatically be closed in two months if no further activity occurs. Thank you for all your contributions.