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

fix: remove sudo from installation script, add bash flag to quit on f… #362

Closed
wants to merge 2 commits into from

Conversation

albjeremias
Copy link

@albjeremias albjeremias commented Nov 3, 2024

…irst error

👋 I did some awesome work for the Pelias project and would love for everyone to have a look at it and provide feedback.


Here's the reason for this change 🚀


Here's what actually got changed 👏

  • remove sudo from the readme installation script
  • add flag to bash -xe so that bash script fails on first error
  • split command && into two lines, to make it more clear

Here's how others can test the changes 👀

create a new pelias installation in a clean computer

@missinglink
Copy link
Member

Hi @albjeremias, using export will only modify the PATH for the current terminal session whereas symlinking it into /usr/local/bin makes the pelias command immediately available in all terminal sessions and will survive restarts (ie. what you'd consider an install in Linux)

@albjeremias
Copy link
Author

Hi @albjeremias, using export will only modify the PATH for the current terminal session whereas symlinking it into /usr/local/bin makes the pelias command immediately available in all terminal sessions and will survive restarts (ie. what you'd consider an install in Linux)

so maybe we can request the user? :) what do you think?
anyway i feel that installing, should really be installing pelias in the system, not just a symlink to a folder with a git repository

@missinglink
Copy link
Member

missinglink commented Nov 4, 2024

so maybe we can request the user? :) what do you think?

Sorry I don't understand your question.

not just a symlink to a folder with a git repository

This is just how Linux works, the software is extracted to a location on the filesystem and the executables are symlinked into the PATH.

If you don't like the installation location you have chosen you can clone into /usr/local or /opt if you prefer, doing so will require use of sudo.

@albjeremias
Copy link
Author

albjeremias commented Nov 4, 2024

so maybe we can request the user? :) what do you think?

Sorry I don't understand your question.

not just a symlink to a folder with a git repository

This is just how Linux works, the software is extracted to a location on the filesystem and the executables are symlinked into the PATH.

If you don't like the installation location you have chosen you can clone into /usr/local or /opt if you prefer, doing so will require use of sudo.

oh sorry i wasn't clear enough.
1st, on this commit i request the user if he wants to use install with sudo or not.

2nd, please, thats not how linux works. if you want to install something you need to copy all files dependent on the bin to /usr/local/lib or else you are creating an insecure symlink to a userspace git repository, thats not safe.

@missinglink
Copy link
Member

missinglink commented Nov 4, 2024

I appreciate where you're coming from but it's important to understand that a large percentage of users have little to no experience with Linux, they often get stuck with permissions issues which they assume are bugs in the software.

request the user if he wants to use install with sudo or not

This installation workflow is simply for absolute beginners to get started, you're free to install it however you wish.

Prompting users to select between adding to their PATH vs. adding to their env is going to be too much for most to understand, the latter in particular which doesn't work in a new terminal or when logging in/out of their SSH connection is going to be super confusing.

I would very much like to avoid any linux related issues on this repo, we Dockerize the tools specifically to try and avoid these.

you need to copy all files dependent on the bin to /usr/local/lib or else you are creating an insecure symlink

This is a fair criticism, I suggested the same thing to you in my previous comment.

In order to move forward, I would appreciate if you could specify specifically what problem you are trying to solve.

@albjeremias
Copy link
Author

I appreciate where you're coming from but it's important to understand that a large percentage of users have little to no experience with Linux, they often get stuck with permissions issues which they assume are bugs in the software.

request the user if he wants to use install with sudo or not

This installation workflow is simply for absolute beginners to get started, you're free to install it however you wish.

Prompting users to select between adding to their PATH vs. adding to their env is going to be too much for most to understand, the latter in particular which doesn't work in a new terminal or when logging in/out of their SSH connection is going to be super confusing.

I would very much like to avoid any linux related issues on this repo, we Dockerize the tools specifically to try and avoid these.

you need to copy all files dependent on the bin to /usr/local/lib or else you are creating an insecure symlink

This is a fair criticism, I suggested the same thing to you in my previous comment.

In order to move forward, I would appreciate if you could specify specifically what problem you are trying to solve.

Hi,
well i already solved my issues, im just trying to support an open source project. if you feel that this are the politics of your project i have nothing new to add, already said everything I had to say. please close the pull request, I won't do it. sorry. have a nice day.

@missinglink missinglink closed this Nov 5, 2024
@missinglink
Copy link
Member

missinglink commented Nov 5, 2024

Thanks for flagging up the permissions issue, I agree it's not ideal to symlink a user-space directory onto the PATH, unfortunately this seems to be the best way of supporting novice users at the moment.

Another option worth considering is to add the export PATH statement to ~/.bashrc or equivalent.

if you feel that this are the politics of your project

I'm not sure what you mean by this?

Asking for clarification & advocating for novice users is not political.

Programming is much more than typing on the computer, there are often many people affected by what seem like innocuous changes.

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