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

scripts: Handle redirects in image URLs #656

Merged
merged 1 commit into from
Aug 10, 2023

Conversation

garrett
Copy link
Member

@garrett garrett commented Aug 9, 2023

GitHub changed the way they handle images; their URLs might not have an extension now. For the time being, we should assume the images without extensions are PNG files.

We will probably want to download whatever they are and run some identification on them in the future (like the file command).

I also switched the string extension and string concat to use native Ruby File.* commands.

@garrett garrett requested a review from jelly August 9, 2023 10:22
@jelly
Copy link
Member

jelly commented Aug 9, 2023

This does not seem to work for me:

Re-added release-note for this image cockpit-project/cockpit#17796 which works but now get's an extra .. the other image is empty.

[jelle@t14s][~/projects/cockpit-project.github.io]%file images/299-networking-add-support-for-wireguard-2.png
images/299-networking-add-support-for-wireguard-2.png: empty
[jelle@t14s][~/projects/cockpit-project.github.io]%file images/299-storage-set-up-a-system-to-use-nbde..png
images/299-storage-set-up-a-system-to-use-nbde..png: PNG image data, 853 x 313, 8-bit/color RGBA, non-interlaced

Copy link
Member

@jelly jelly left a comment

Choose a reason for hiding this comment

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

This does not seem to be fixable by this script, we can't get the images even if we hack the url:

Working old url:

https://user-images.githubusercontent.com/3228183/211269003-613885c7-6a11-4416-b69d-6435560ec49a.png

New url:

https://github.com/cockpit-project/cockpit/assets/200109/bb33f362-a4ac-470e-a9b9-62ee5c3d3116

Modify into user-images url just fails so ugh not much we can do.

curl https://user-images.githubusercontent.com/200109/bb33f362-a4ac-470e-a9b9-62ee5c3d3116.png
<?xml version="1.0" encoding="UTF-8"?>
<Error><Code>AccessDenied</Code><Message>Access Denied</Message><RequestId>TRQ7PN4ARBTBNPFN</RequestId><HostId>x8JWBK0ueMfd6TzZK5GVAi4XiYSUGkr/mtve1V/z120GXG5ACa7z32fGDLVkL0B5PTnumegDPiY=</HostId></Error>%

_scripts/generate-release-notes.rb Outdated Show resolved Hide resolved
_scripts/generate-release-notes.rb Outdated Show resolved Hide resolved
@garrett garrett force-pushed the scripts-release-extension branch 2 times, most recently from 429e753 to 39ed663 Compare August 9, 2023 13:31
@garrett garrett changed the title scripts: Handle images without an extension scripts: Handle redirects in image URLs Aug 9, 2023
@garrett
Copy link
Member Author

garrett commented Aug 9, 2023

Addressed your issues and integrated your suggestions @ #657, but added multiple redirect support (recursive calling based on response) and error handling.

If this is merged, it:

closes #657

@garrett garrett force-pushed the scripts-release-extension branch 2 times, most recently from cdf0752 to 5d13491 Compare August 9, 2023 13:50
@garrett garrett force-pushed the scripts-release-extension branch from 5d13491 to 8c76395 Compare August 10, 2023 08:18
Copy link
Member

@jelly jelly left a comment

Choose a reason for hiding this comment

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

Would have liked to have a comment about the new urls which we need to handle now but let's get it merged. 👍

@jelly jelly merged commit 76af5e1 into cockpit-project:main Aug 10, 2023
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.

2 participants