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

Create A Pull request #515

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Create A Pull request #515

wants to merge 7 commits into from

Conversation

CooperDActor
Copy link

@CooperDActor CooperDActor commented Apr 27, 2024

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

Copy link
Contributor

@necouchman necouchman left a 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)
Copy link
Contributor

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?

Copy link
Author

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?

Copy link
Contributor

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.

Depend-on/Bash.sh Show resolved Hide resolved
Depend-on/Bash.sh Show resolved Hide resolved
echo "Neither apt-get nor dpkg found. Unsupported package manager."
fi
;;
*)
Copy link
Contributor

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.

Copy link
Author

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

Copy link
Contributor

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
Copy link
Contributor

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 ;-).

Copy link
Author

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

Copy link
Author

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

Depend-on/Bash.sh Show resolved Hide resolved
Depend-on/Bash.sh Show resolved Hide resolved
@CooperDActor
Copy link
Author

@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).---- under issues get me a t-issue okay if you dont like that joke u have a cold heart but really under issues or where?? thats why I wasn't doing it
  • 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. --- this is not an insult but think of better names here's how I think of it contrib wait contribute maybe someone put malicious code In it NUP NUP NUP just like my mother I could put dependencies but autocorrect saved me there
  • 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.---- okay
  • 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" -- next time or if I can I will edit it
  • 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/.-- will do from now on
  • Finally, 7 commits for a single shell script seems a bit excessive - you might consider squashing down to one or two commits. ----- I made it simple first time they wanted more if you can as I dont know how. you could squash it down (adding you to the contrib on my guac folder so you can edit) if you need/want

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