-
Notifications
You must be signed in to change notification settings - Fork 501
Correctly post GitHub commit status for out-of-repo deployment #2816
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
Correctly post GitHub commit status for out-of-repo deployment #2816
Conversation
At the moment `post_status` assumes source and deploy repos are the same, it's even completely ignoring the argument which holds the source repo.
| Sys.which("curl") === nothing && return | ||
| ## Extract owner and repository name | ||
| ## Extract owner and repository names | ||
| source_owner, source_repo = split(source, '/') |
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 there any chance gha.github_repository includes the .git extension? In that case I'd need to drop it with a regex like done 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.
Hopefully not
Documenter.jl/src/deployconfig.jl
Line 323 in 5da6aa3
| github_repository = get(ENV, "GITHUB_REPOSITORY", "") # "JuliaDocs/Documenter.jl" |
.git.
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 think it's fine to assume no .git.
src/deployconfig.jl
Outdated
| push!(cmd.exec, "https://api.github.com/repos/$(source_owner)/$(source_repo)/statuses/$(sha)") | ||
| # Run the command (silently) | ||
| io = IOBuffer() | ||
| @debug "About to run curl command" cmd |
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.
This was helpful for me for debugging: #2814 (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.
This would print the token though, so I am a little bit hesitant to add this.
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 maybe remove this? Or refactor, so that we push! the token after we @debug it? Otherwise, I have no reservations and the PR LGTM.
| @debug "About to run curl command" cmd |
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.
This would print the token though
Secrets are never printed to screen, they're replaced with asterisks, as in the example I linked above.
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.
Only if you're running things in the context where that is true (like on GHA, where the token is a secret). I'd just prefer to be defensive here and never print the secret.
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.
Ok, done.
|
@milankl, with this PR Documenter should show the deploy-docs status even when Documenter pushes on a different repo. |
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.
I don't claim to understand the details, but (a) I trust @giordano to know what he's doing, and (b) it was tested and worked, so I'd be willing to give it a go
The |
|
This is great, thanks @giordano for your work on this: To clarify, would this deploy docs check automatically pop up when a new version of Documenter is released and used in a repo that deploys docs to another repo? Happy to report back here whether this works but do I need to do anything 😉 ? |
Yes.
No, just wait for next release. |
At the moment
post_statusassumes source and deploy repos are the same, it's even completely ignoring the argument which holds the source repo.I don't know how to test this in CI, but I did test it in NumericalEarth/Breeze.jl#77 (comment).
Fix #2814.