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

Made the script Docker-compatible + Prevented version failures #690

Merged
merged 4 commits into from
Jan 7, 2019
Merged

Made the script Docker-compatible + Prevented version failures #690

merged 4 commits into from
Jan 7, 2019

Conversation

angeryrohan
Copy link

@angeryrohan angeryrohan commented Dec 22, 2018

Description

Made setup.sh compatible for Docker and bash users:

Docker always runs as root, Earlier the "sudo" commands were making trouble for Docker but not for bash users. Docker can't access the sudo command as its $USER is already set as root. In this commit, I've removed all the sudo (s) in the shell script.

OK, but wouldn't this mess up the installation for bash users since they require some permission to install packages?


To take care of that, I added an If-else condition for detecting the $USER, If that's found as a "non-root" user, An echo of instructions to proceed with root user is given out in the terminal, When bash users switch to SuperUser (su) and execute this script, the installation takes place flawlessly.

EXAMPLE: WHEN ACCESSING SCRIPT AS "NON ROOT USER"

proo


EXAMPLE: WHEN ACCESSING SCRIPT AS "ROOT USER"

proof

NOTE: This implementation forces bash users to use SU, that's because linux bash users can compromise, but as for docker users, They can only install packages as root and using sudo in the script for docker would be making trouble. So I implemented a way which could support installation for both, bash and Docker users. For the current implementation, The Please run as root to proceed with installations won't ever come in Docker because it only runs as a root user

Prevented version failures

It's important to first update package list, then update the versions of all pre-installed packages in the system. This script installs a few popular things like npm,pip,nodejs etc..
Many people like me are already having them. In that case, APT just ignore the installation if the presence of that specific package is detected, and this results in the operations of older versions of packages, which to an extent may cause depreciation warnings or failures. If we update and upgrade the versions of the packages, The build gets cleaner and safer. I've added two shell commands before the installation:
1. apt-get update: To update package list, this is a good practise to update the list before installing any packages.
2. apt-get upgrade: To get all the pre-installed packages on the latest version.

Related Issue

#245

Motivation and Context

This change is required to make this installation script compatible for Docker users

How Has This Been Tested?

This has been tested by executing the script in the terminal with 2 different users:

  1. root users
  2. non-root users

Screenshots (In case of UI changes):

Uploaded Above ^

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Docker always runs as root, Earlier the "sudo" commands were making trouble for Docker but not for bash users. Docker can't access the sudo command as its $USER is already set as root. In this commit, I've removed all the sudo (s) in the shell script while ensuring support for docker users...
@angeryrohan
Copy link
Author

@OshanIvantha @agentmilindu @malithsen , The issue #245 is covered through this pull request.
All code analyses tests are now passing. The setup.sh script now runs for both direct-bash and docker users. Please review
Thank You ^.^

setup.sh Outdated
if [ $USER = "root" ]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Username can be anything which has root privileges

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, What could we do for that?

Copy link
Author

@angeryrohan angeryrohan Dec 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have an alternative. Please give your opinions on this @malithsen @vivonk

My current PR would only work for default "root" username, Although @malithsen @vivonk , If a custom username say "XYZ" is being used with root privilages. Then the shell script would show the user:
Please run as root to proceed with Installations...

Let's change this step to show:

Please run as default root to proceed with installations

and we have already provided the steps below this line to execute commands with default root user below it which I believe, are very simple steps. Yes I completely agree with @malithsen , it increases the complications, but its better to compromise on ease of installations than to compromise on accessibility for docker users
^.^

@malithsen
Copy link
Collaborator

Good work @rohancl
I am a bit worried about the complications this may cause for logging. Bassa made to be working in a corporate environment. When you switch to root with su, Bassa's logging will always see the running user as root. This will not happen when we use sudo.

This is not a major concern because I don't think we are/will log a lot of user level stuff. Unless there are better suggestions, let's go with this PR :)

@malithsen
Copy link
Collaborator

How about using

if ! [ $(id -u) = 0 ]; then
   echo "Not root. Run normal commands"
else
   echo "root. Run docker commands"
fi

id -u is only zero when running under an account with root privileges. And it works fine with sudo eliminating the need to use su. I haven't tested this extensively though. @rohancl wdyt?

@angeryrohan
Copy link
Author

How about using

if ! [ $(id -u) = 0 ]; then
   echo "Not root. Run normal commands"
else
   echo "root. Run docker commands"
fi

id -u is only zero when running under an account with root privileges. And it works fine with sudo eliminating the need to use su. I haven't tested this extensively though. @rohancl wdyt?

Yeah that's awesome, Although, that would require us to keep 2 different shell scripts.
1 for normal users
1 for docker users
Overall, That's an add on, but much more stable...
What do you think @vivonk ?

@angeryrohan
Copy link
Author

Wait, I think I should try using your code in my way.
I'll edit and make a commit in few hours ...

@angeryrohan
Copy link
Author

angeryrohan commented Dec 27, 2018

@agentmilindu @malithsen @vivonk Check the script now. I think this way of detection provided by @malithsen is more efficient, Earlier I used a static approach of detecting the root and non-root users, this method is dynamic.

Copy link
Collaborator

@vivonk vivonk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM now. Good work man

@angeryrohan
Copy link
Author

Thanks @vivonk ^.^

#!/bin/sh
if ! [ $(id -u) = 0 ]; then
printf "Please run as root to proceed with installations... \n1. Type in: su\n2. Enter your password\n3. Execute: sh setup.sh\n"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason for using printf instead of echo ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codacy code quality requirement
There is no output difference btw on using echo or printf ^.^

@agentmilindu agentmilindu merged commit 72f8c55 into scorelab:develop Jan 7, 2019
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.

5 participants