progit / progit2

Pro Git 2nd Edition
Other
5.79k stars 1.9k forks source link

GitHub Action release workflow doesn't provide contentType #1673

Open HonkingGoose opened 3 years ago

HonkingGoose commented 3 years ago

Which version of the book is affected?

Problem is with source files on main. More specifically this file: https://github.com/progit/progit2/blob/main/.github/workflows/release-on-merge.yml

Describe the bug:

We recently migrated from Travis CI to GitHub Actions. The action we use to create our release (https://github.com/ncipollo/release-action) was causing issues with the generation of the links to the downloadable files over on the git-scm.com side.

@peff made changes over on the git-scm.com side to find the files based on the filename suffix. To be clear, we're not having urgent problems right now anymore, but we should probably still clean up this problem. 😄

@peff recommended that we either:

  1. Help fix upstream action so that it gets a way to provide the content type.
  2. Select different action that provides the content-type.

Copy/paste of relevant quotes from @peff (chronological order):

Initial discovery of broken download links

Hmm, now we don't seem to be generating links for any books! We find the appropriate assets for each release based on their content_type field. But that has changed recently. Running this ruby snippet:

require "octokit"
o = Octokit::Client.new(access_token: ENV["GITHUB_API_TOKEN"])
o.releases("progit/progit2").each do |rel|
        rel.assets.each do |asset|
                puts "#{rel.tag_name} #{asset.name} #{asset.content_type}"
        end
end

yields:

2.1.313 progit.epub raw
2.1.313 progit.html raw
2.1.313 progit.mobi raw
2.1.313 progit.pdf raw
2.1.312 progit.epub raw
2.1.312 progit.html raw
2.1.312 progit.pdf raw
2.1.311 progit.epub raw
2.1.311 progit.html raw
2.1.311 progit.pdf raw
2.1.310 progit.epub raw
2.1.310 progit.html raw
2.1.310 progit.pdf raw
2.1.309 progit.epub raw
2.1.309 progit.html raw
2.1.309 progit.pdf raw
2.1.308 progit.epub application/epub+zip
2.1.308 progit.html text/html
2.1.308 progit.pdf application/pdf
2.1.307 progit.epub application/epub+zip
2.1.307 progit.html text/html
2.1.307 progit.pdf application/pdf
2.1.306 progit.epub application/epub+zip
2.1.306 progit.html text/html
2.1.306 progit.pdf application/pdf
2.1.305 progit.epub application/epub+zip
2.1.305 progit.html text/html
2.1.305 progit.pdf application/pdf
[...and so on...]

So it looks like something changed recently on the release-generation side, and the content-types were lost. We probably could switch to matching the extension in the filename, but I suspect it would be worth fixing the content-type problem for other reasons (e.g., I imagine it impacts the content-type GitHub sends to users when they click on the releases in a browser).

Figuring out what changed (different release provider)

So it looks like something changed recently on the release-generation side, and the content-types were lost.

Diffing 2.1.308 and 2.1.309, it looks like the release creation now uses https://github.com/ncipollo/release-action, but doesn't pass artifactContentType, so the default is raw. It's not clear to me if that property can take a comma-separated list like artifacts does.

Futher debugging

It's not clear to me if that property can take a comma-separated list like artifacts does.

Looks like no. The code seems to apply a single content-type to every artifact. So a fix would probably involve extending that action, and then updating progit2 to make use of the new feature. Yikes.

Even if that happens, it may take a while. I think we should switch to selecting based on the names in the meantime.

Steps to reproduce:

I think @peff has made it clear what changed with their log output from using the octockit script.

Expected behavior:

Our release action provides the correct content type.

Screenshots:

Additional context:

I'll link the start of the discussion of the ProGit2 build process over on the git-scm.com side: https://github.com/git/git-scm.com/issues/1498#issuecomment-870817638