Skip to content
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

Use current GHA ref instead in of HEAD #2623

Merged
merged 8 commits into from
Jun 3, 2024
Merged

Use current GHA ref instead in of HEAD #2623

merged 8 commits into from
Jun 3, 2024

Conversation

hadley
Copy link
Member

@hadley hadley commented Jun 3, 2024

@olivroy while I was working on this I realised it's not actually the default branch that you want to link to, but the "current" branch, i.e. the source links in a pull request should link to that corresponding branch. It took me a while to figure out how to do this, but I think this system of GitHub env vars should be robust.

Fixes #2597

Copy link

github-actions bot commented Jun 3, 2024

@hadley hadley changed the title Use branch name instead in of HEAD Use current GHA ref instead in of HEAD Jun 3, 2024
@hadley hadley requested a review from olivroy June 3, 2024 21:26
Copy link
Collaborator

@olivroy olivroy left a comment

Choose a reason for hiding this comment

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

Looks good!

If your change to DESCRIPTION is required, better to document() before merging?

I checked the PR preview, and the link worked out of the box! So that is super nice! The link will likely not work for external PRs generally, but that's okay since we'll just want to ensure that the correct branch is picked up on released sites.

I will check post-merge to make sure the links are correctly connected to main for pkgdown (not gh-pages) !

DESCRIPTION Outdated Show resolved Hide resolved
Co-authored-by: olivroy <52606734+olivroy@users.noreply.github.com>
@olivroy
Copy link
Collaborator

olivroy commented Jun 3, 2024

Maybe one thing I am worried about. For persons who still use the docs folder.

For example, I sent this PR a few months ago cjvanlissa/worcs#140

I built the site locally and sent an external PR. This PR would probably have created bad links if I had done it using pkgdown 2.0.9.9000 with this?

It seems like you avoid this issue. I may do a bit of integration testing prior to release post-merge!

@hadley
Copy link
Member Author

hadley commented Jun 3, 2024

@olivroy the behaviour shouldn't change for local builds; it's only when running in GHA you'll get different links. So I think this should be safe.

@hadley hadley merged commit 6d79f4e into main Jun 3, 2024
14 checks passed
@hadley hadley deleted the default-branch branch June 3, 2024 22:21
@olivroy
Copy link
Collaborator

olivroy commented Jun 4, 2024

@hadley

Awesome, It correctly detected main and the link is correct in https://pkgdown.r-lib.org/dev/articles/pkgdown.html

maybe one last consideration:

Update a released site via workflow dispatch: If you create a new branch to backport changes, update site and then delete the branch, the links won't be correct?

Is it worth documenting this in how-to-update-released-site vignette?

Also, with dev mode = "auto", will this work too on release? or wull it create a link based on tag? In this case, it is more appropriate to target default branch, to have the smooth workflow I suggested in #2597

@hadley
Copy link
Member Author

hadley commented Jun 4, 2024

I mean the links will be technically correct in that they'll point to that temporary release branch. And similarly for the release site; like in some way it's arguably correct that they point to the tag at the time of release (since it might be different in dev). I think we can just let it sit for a while and see how it feels.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Could pkgdown have a way to detect default branch?
2 participants