-
-
Notifications
You must be signed in to change notification settings - Fork 450
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
Add utility functions for processing Github releases #330
base: main
Are you sure you want to change the base?
Add utility functions for processing Github releases #330
Conversation
…om github and untars it. Uses curls and standard gnu tools.
Well meant, but i think, it will probably not work. (ive not tested it yet) In addition, not all scripts are moved/copied to /opt/LXC, but there are also downloads that are located in /usr/bin or /etc. |
The idea is sound, but in my experience there's always a few repos that work differently. So this would work for maybe 50% (or more) of our cases, but definitely not all of them. Plus, we are not optimizing much, like actual code for most script is just 3-4 lines:
|
Following up on this, for me, comprehending these 5 commands is much easier and quicker than having a custom function which I have to find and understand input/output/logic/error handling etc. |
Thanks for the feedback. I hear what you all are saying about verbosity of the code. I intentionally added verboseness to make it easier to read, and error messages that could be removed. The reason I am suggesting this is so that we can reduce the amount of code in the scripts, much like the check resources functions that were added. It gives community script writers a framework on which they can and must adhere to, which means less reviewing for us. The classic 5 liner @remz1337 and @havardthom mentioned, is not hard to read, but can contain subtle errors intentional or otherwise. Consider this below, can you spot the problem immediately (do not run this code!!!)? Even if you can quickly spot it, can you trust every reviewer will also spot it?
I have also been writing a github action (will get some help from @havardthom ) to catch these kind of security problems. The 5 line pattern could be reduced to github_latest_release_extract "goauthentik" "authentik" "/opt/authentik" There is very little possibility that this could contain any hidden flaws or features. It would also allow us in the future to audit the scripts in an automated manner. I will modify my pull request to simplify the code. |
It would not matter. The reason I broke it up into two functions was so that if we wanted to do something more complicated with the tar, we could just use the download function. As I mentioned in my comment above, I will modify this code with feedback. |
If we are using the latest github release to install and update, I don't think it can work differently. If the project does not use releases, then of course it wouldn't be applicable. For example if they have a stable branch rather than a formal release. This would be applicable to a lot of current scripts and a significant number of future scripts. Number of files containing 'github.com/.*/releases': 71 So at least 39% of current scripts.
|
I have significantly simplified the functions, taking into account the feedback here, including removing error messages and using only wget, not curl, to make things more consistant. You can test with this example script:
|
If add this should be used with caution. By defaulting to latest we risk pulling a version that breaks our install script. I am a fan of pinning it to a major release ^2.0.0. |
@judeibe I have made the case for pinning but the general consensus has been it would create too high of a maintenance burden. Many if not most of the scripts install the latest version. The update functionality certainly does. |
You have good arguments and I'm starting to agree with your main point of having a utility function to abstract these install parts, but this adds a utility function that not cover a majority of the scripts since it only handles repo source code / tarball_url. It will add another way of downloading from github, breaking the existing conventions, possibly causing confusion on when to use what. It could also incentivize building from source code which is not preferable if there are linux releases or distribution packages in the github release. Looking through the 30 first scripts I found these that the utility function would not support and would still need to use current convention.
It would also break the current regime on updating, where we store the release version in a txt file which is used to check when an update is available. I guess the main question I have is if it can be extended to handle all our cases without being to complicated? I'm not so sure about that, and if not, it will add more confusion than value i think |
@havardthom thanks for the feedback. I will take a look at different and “edge” (maybe unique is a better word) cases and see if they could be supported. I see your point regarding binary releases. I think there is value in standardising future scripts even if current scripts are not updated or are only updated over time. |
I believe applying new standards only to future scripts would risk breaking one of the fundamental advantages of this project, where you can (relatively) easily see how a script should be made just by browsing the existing ones. For me, new standards should be able to be applied to a majority of scripts including the current ones (over time). |
Could we also/ at least have a function that just gets the latest release tag? Just search for "tag_name" (https://github.com/search?q=repo%3Acommunity-scripts%2FProxmoxVE%20tag_name&type=code) which is used in about a 100 places in the repo for version tagging (like @havardthom mentions) and people use different implementations to do the same thing. |
@havardthom please check the link I posted above, practices around Github release tags are quite diverse, and it is unclear what "the convention" is; I happened to copy the version in my outline script from the linkwarden script, and that version seems also quite common. I'm happy that @MickLesk is working on a template/standard/guidelines, I think those will make everyone's life easier since it's easier to work from a template/standard than trying to figure out the conventions by searching for commonalities across hundreds of scripts. |
Note
We are meticulous when it comes to merging code into the main branch, so please understand that we may reject pull requests that do not meet the project's standards. It's never personal. Also, game-related scripts have a lower chance of being merged.
Description
Many scripts download github releases. This PR adds two utility functions:
download_latest_github_release(user, repo) downloads the latest release and returns (echos) the filename. It relies on the github HTTP API.
extract_github_tarball(tarball, directory) untars a github release into target directory (this is regardless of how the release is named).
This could be used to fix issue #321.
Because this modifies install.func it should be considered high risk.
These functions can be easily test, for example:
Type of change
Please check the relevant option(s):
Prerequisites
The following efforts must be made for the PR to be considered. Please check when completed: