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

remove sudo calls as script must be run as root #80

Closed
wants to merge 1 commit into from

Conversation

@tmornini
Copy link

@tmornini tmornini commented Sep 23, 2015

This patch eliminates:

sudo: sorry, you must have a tty to run sudo

exceptions when configuring Loggly via an AWS EC2 user data script.

Since checkIfUserHasRootPrivileges() on https://github.com/loggly/install-script/blob/master/Linux%20Script/configure-linux.sh#L190 requires script to be run as root the use of sudo is entirely superfluous. :-)

Until this patch is accepted, we're correcting in the user data script via:

sed -i.original 's/sudo //' configure-linux.sh
@tmornini
Copy link
Author

@tmornini tmornini commented Oct 6, 2015

Ping

@mostlyjason
Copy link
Contributor

@mostlyjason mostlyjason commented Nov 17, 2015

@psquickitjayant can you review this one? Probably good for you to watch this repo too. I noticed some formatting changes.

@tmornini
Copy link
Author

@tmornini tmornini commented Nov 17, 2015

@mostlyjason Hurray! Thanks for paying attention. :-)

@tmornini
Copy link
Author

@tmornini tmornini commented Dec 22, 2015

Ping

@varshneyjayant
Copy link
Contributor

@varshneyjayant varshneyjayant commented Dec 23, 2015

sorry I missed it. I will check it now :)

@tmornini
Copy link
Author

@tmornini tmornini commented Feb 4, 2016

@psquickitjayant Ping

@varshneyjayant
Copy link
Contributor

@varshneyjayant varshneyjayant commented Feb 5, 2016

@tmornini I have added this in my backlog. I am doing changes in the script. Once done, then will merge you changes.

@tmornini
Copy link
Author

@tmornini tmornini commented Feb 5, 2016

@psquickitjayant Are you joking? This has been outstanding since September!

Why don't you merge the patch and rebase your changes atop master?

@mostlyjason
Copy link
Contributor

@mostlyjason mostlyjason commented Feb 9, 2016

How does this work for users who are not on EC2, for example if they are running as a non-root user on their own linux installation? It seems we need something that works for them too. If there is a way, we'd have to test it on other distributions too. Also, do we need the formatting changes? It seems better to follow a consistent standard.

@tmornini
Copy link
Author

@tmornini tmornini commented Feb 12, 2016

@mostlyjason Please read PR description again.

This script REQUIRES that it be run as root. As such, there's zero need to use sudo. :-(

@mostlyjason
Copy link
Contributor

@mostlyjason mostlyjason commented Mar 30, 2016

@psquickitjayant is this deployed now? if so, we can close

@varshneyjayant
Copy link
Contributor

@varshneyjayant varshneyjayant commented Mar 31, 2016

@jmckee46
Copy link

@jmckee46 jmckee46 commented Mar 31, 2016

@mostlyjason @psquickitjayant This repository is out-of-date, i.e. the Linux configure script does not match what's on Github, though it does include the removal of sudo as shown by this PR.

[ec2-user@ip-10-0-2-107 test]$ curl https://www.loggly.com/install/configure-linux.sh | sha256sum 
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 25960  100 25960    0     0  48265      0 --:--:-- --:--:-- --:--:-- 48342
9ff4c007070199f6da53077c889ea4ba8f48557a2cab9fba197f2be63ee1127d  -
[ec2-user@ip-10-0-2-107 test]$ curl https://raw.githubusercontent.com/loggly/install-script/master/Linux%20Script/configure-linux.sh | sha256sum 
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 25620  100 25620    0     0  79333      0 --:--:-- --:--:-- --:--:-- 79318
0bbce5ad2dbdd2a62785d789f790163f2e17b26310710e1fa4dcd70c1e01fd99  -
@jmckee46
Copy link

@jmckee46 jmckee46 commented Mar 31, 2016

Wow, the script at https://www.loggly.com/install/configure-linux.sh not only differs with respect to sudo handling, but it entirely changes the semantics of the script in at least two ways:

  1. It no longer runs verification as it used to.

  2. APP_TAG is now destroyed on line 21, making it impossible(?) to set APP_TAG.

APP_TAG=
@mostlyjason
Copy link
Contributor

@mostlyjason mostlyjason commented Apr 14, 2016

@psquickitjayant can you test and confirm what @jmckee46 says? We want verification and the tags to work.

@varshneyjayant
Copy link
Contributor

@varshneyjayant varshneyjayant commented Apr 18, 2016

@mostlyjason @jmckee46 APP_TAG is a property which is used by the other scripts like apache, tomcat, etc.

It does not go for the verification steps if there are not any changes in the 22-loggly.conf file and it already exists.

If you do a fresh install, or if there are any changes in 22-loggly.conf then verification steps run.

@mchaudhary
Copy link
Contributor

@mchaudhary mchaudhary commented May 4, 2016

We have already made the changes to remove the sudo from the script and make sure the script is run as root.

@mchaudhary mchaudhary closed this May 4, 2016
@tmornini
Copy link
Author

@tmornini tmornini commented May 4, 2016

Wow, fantastic, only took 1 month for first response and 7 months to close.

And no merge, giving no credit or thanks for my pull request.

You folks are truly living the open source dream! :-(

@mostlyjason
Copy link
Contributor

@mostlyjason mostlyjason commented May 4, 2016

@tmornini sorry about that! Thanks for your contribution! I think our forks are out of sync so we merged it in https://github.com/psquickitjayant/install-script. I'm not sure which commit it was in though. Hopefully we'll get this one synced up soon. Let us know if anything is missing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.