frankenphp icon indicating copy to clipboard operation
frankenphp copied to clipboard

Feature: Add one line install script and instructions to README

Open kevincobain2000 opened this issue 1 year ago • 13 comments

Install cli could be more convinient for for users to not guess b/w arm, x86_64 and aarch etc.. Plus it will provide better installation for dev-ops team as one interface.

Verify

╰─$ cat install.sh| sh -
######################################################################## 100.0%
Installed successfully to: /Users/pulkit.kathuria/git/action-coveritup/app/frankenphp

╰─$ ls -lh frankenphp
-rwxr-xr-x  1 user.name  679754705   105M Feb 25 20:15 frankenphp

kevincobain2000 avatar Feb 25 '24 11:02 kevincobain2000

Looks like some issues with shellcheck in GHA (mostly variables need to be quoted). Can you address that?

withinboredom avatar Feb 26 '24 17:02 withinboredom

Thanks for the review @withinboredom I have fixed the lint errors on spellchecker, I think rest of the lint errors are un-related to changes in this pull request. May want to re-run the CI.

kevincobain2000 avatar Feb 27 '24 02:02 kevincobain2000

Ideally it'd be also nice to have this script hosted on https://frankenphp.dev/install.sh instead of raw.github... But that is topic for another day - and may depend on how is the docs pipeline structured.

kevincobain2000 avatar Feb 27 '24 02:02 kevincobain2000

There are still remaining issues (double quoting to prevent globbing). The paths should be quoted because if my current pwd is /home/withinboredom/bin*2024 it will fail in fun ways trying to download the file.

withinboredom avatar Feb 27 '24 07:02 withinboredom

Squashed commits and lints fixed.

kevincobain2000 avatar Feb 27 '24 10:02 kevincobain2000

Is this safe?

Composer uses a checksum for instance: https://getcomposer.org/download/

dunglas avatar Feb 27 '24 16:02 dunglas

Is this safe? Composer uses a checksum for instance: https://getcomposer.org/download/

No comments on this. I mean I guess you could add this extra layer of security and have checksum. At the same time I also don't see the point as it is not a package manager. Homebrew installation doesn't have similar either and not many other curl shell installations.

kevincobain2000 avatar Feb 28 '24 01:02 kevincobain2000

I also don't see the point

Incomplete downloads, bad actors/broken caches on corporate middle-boxes, and a host of other things can cause checksums to be invalid. It's worth detecting it if we can, if for no other reason than to save people some time when things aren't working as expected.

withinboredom avatar Feb 28 '24 11:02 withinboredom

It is worth detecting. Yes. Just for this install script it seems over work. And may not have a simple straightforward command for the user. It’s a trade off.

kevincobain2000 avatar Feb 28 '24 12:02 kevincobain2000

Ok, that works for me. That would be nice to have an official brew and maybe APT repositories too (easier to update).

dunglas avatar Feb 28 '24 14:02 dunglas

Yes. Given the popularity of this tool it’d be at least wise to reserve the names on brew and/or apt get etc. Publications can be delayed when you have time. Suggestion is to at least own the name before some bots started claiming it. It is just a naive thought.

kevincobain2000 avatar Feb 28 '24 14:02 kevincobain2000

Hello,

They are remaining tasks to do before merging this PR ? Would be great to have this in the README.md file

SirMishaa avatar Apr 14 '24 18:04 SirMishaa

I believe this is ready. All changes from my side have been pushed.

kevincobain2000 avatar Apr 22 '24 13:04 kevincobain2000