Skip to content

scripts: Handle redirects in image URLs#656

Merged
jelly merged 1 commit into
cockpit-project:mainfrom
garrett:scripts-release-extension
Aug 10, 2023
Merged

scripts: Handle redirects in image URLs#656
jelly merged 1 commit into
cockpit-project:mainfrom
garrett:scripts-release-extension

Conversation

@garrett
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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>%

Comment thread _scripts/generate-release-notes.rb Outdated
Comment thread _scripts/generate-release-notes.rb Outdated
@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
Copy Markdown
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
Copy Markdown
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