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

SQSCANGHA-55 Support GitHub self-hosted runners without wget #151

Merged
merged 2 commits into from
Nov 28, 2024

Conversation

antonioaversa
Copy link
Contributor

@antonioaversa antonioaversa commented Nov 26, 2024

Replaces wget with curl.

The base of this PR is #148, as it builds on top of it.

The PR is kept in draft to avoid accidental merging.

UPDATE: #148 has been merged, so now this PR directly targets dev.

@antonioaversa antonioaversa force-pushed the antonio/support-for-missing-wget branch 8 times, most recently from 4b7acad to e7a41e6 Compare November 26, 2024 19:05
@antonioaversa antonioaversa requested review from Godin and a team November 26, 2024 19:06
Copy link
Member

@henryju henryju left a comment

Choose a reason for hiding this comment

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

Are we sure curl is always available? Or is it always available when wget is? If not, why not keep both, trying to use one and falling back on the other if the first one is unavailable?

@antonioaversa
Copy link
Contributor Author

antonioaversa commented Nov 27, 2024

Are we sure curl is always available? Or is it always available when wget is? If not, why not keep both, trying to use one and falling back on the other if the first one is unavailable?

Thanks for the input!

We quickly discussed it in a standup of the Mobile Squad few days ago, and decided to just replace wget with curl given its alleged larger availability, and to avoid complexifying the logic. However, while curl is available on all GitHub-hosted runners and on Docker images such as golang (base image of the user who had missing wget - tested here), we cannot be sure that curl is always available on self-hosted runners, nor when wget is.

I have made the changes to support the fallback here. They don't actually complexify the code that much and make the action more robust, so I think it makes sense to go for the fallback.

We will discuss during the standup and come back to you.

@Godin WDYT?

Base automatically changed from antonio/binaries-url-config to dev November 28, 2024 07:06
@antonioaversa antonioaversa marked this pull request as ready for review November 28, 2024 07:07
@antonioaversa antonioaversa force-pushed the antonio/support-for-missing-wget branch from 51a9d67 to a9c2193 Compare November 28, 2024 07:11
Copy link
Member

@henryju henryju left a comment

Choose a reason for hiding this comment

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

Very nice!

@antonioaversa antonioaversa merged commit 1f659fa into dev Nov 28, 2024
25 checks passed
@antonioaversa antonioaversa deleted the antonio/support-for-missing-wget branch November 28, 2024 09:32
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