-
Notifications
You must be signed in to change notification settings - Fork 642
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
Create A Pull request #515
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CooperDActor :
Thanks, again, for jumping in and contributing.
You will definitely need a Jira ticket for this. Creating an account and ticket is pretty easy:
- Go to our Jira page (https://issues.apache.org/jira/projects/GUACAMOLE/issues).
- At the top of the page, click the "Self serve sign-up page" link and request an account.
- Once your account is created, log in and create the Jira issue.
- Once you've created the issue, update the pull request and commit messages with the Jira issue (GUACAMOLE-XXXX:).
Beyond that, I have some comments below for the actual script. And, in general, I still have a couple of concerns about this:
- I'm not crazy about the name
Depend-on/Bash.sh
- first, IMHO, everything should be lower-case, and, second, I'd much prefer a directory called "contrib" or something like that if we're going to start collecting items like this. I can imagine that there may be other things, like RPM spec files, that would fit into this category of items that people might want to work on and contribute to the project, and I'd rather create a broader place to catch that information. - You'll also need to add the Apache 2.0 license header to the top of the shell script - see https://github.com/apache/guacamole-server/blob/main/src/guacd-docker/bin/build-all.sh as an example.
- The title of the pull request should be descriptive of what the changes are you are trying to make - for example, something like "Create a shell script to help manage build dependencies"
- The commit messages should also be descriptive and helpful in determining what has changed. See https://www.codelord.net/2015/03/16/bad-commit-messages-hall-of-shame/.
- Finally, 7 commits for a single shell script seems a bit excessive - you might consider squashing down to one or two commits.
identify_and_run() { | ||
os=$(uname) | ||
if [ "$os" = "Linux" ]; then | ||
distro=$(lsb_release -si) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the lsb_release
command is unavailable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well they're screwed for the moment in future I will test with all operating systems but maybe now we could keep it like that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if the plan is to stick with lsb_release
for this detection, this just highlights the other comment I made - that the default case for the subsequent case
statement should not be to assume that SuSE is in use, but to provide an error that tells the user something useful: "Your distribution was not detected, please make sure your distribution supports the lsb_release command" - or something like that.
echo "Neither apt-get nor dpkg found. Unsupported package manager." | ||
fi | ||
;; | ||
*) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think making the assumption that anything that isn't Ubuntu or EL is automatically SuSE is the right thing to do. While EL-based, Debian-based, and SuSE-based covers most available distributions, there are others out there (like Alpine, which is what we use for the Docker image) and other ways of managing packages.
If you/we don't want to build out and maintain package commands for each of these, that's fine, but then the *)
should throw an error, not assume that everything not EL or Ubuntu-based is SuSE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any thing simpler for my 12yo brain to understand?
what I believe your talking about is the pkg managers and not working if it errors all of the time
see the redundancy talk of my idea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My preference would be to create a "SuSE" section of the case
statement above this *
(default) handler, and put the SuSE commands in that section, and then use the *
area to provide feedback that the distribution is either undetected or unsupported. Roughly, something like this:
case "distro" in
CentOS|RedHatEnterpriseServer|Rocky)
// Run EL commands here
Debian|Ubuntu|Xubuntu)
// Run Debian commands here
SuSE)
// Run SuSE commands here
*)
// Throw error output and exit - distro not detected, not supported, etc.
esac
|
||
|
||
#done | ||
#Rick Astley |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We try not Rick-roll people as part of the source code ;-).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for noticing its like a little easter egg lol but if I cant then I won't put rick Astley
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will put his video there nah when you say no I will follow that to the T
|
I made an easy installer you just have to do this
cd apache-guacamole
cd depend-on
bash Bash.sh
I couldn't create a jiva ticket too hard for a 12yo
thanks Cooper D'Andilly 12yo