Skip to content
This repository has been archived by the owner on Apr 14, 2021. It is now read-only.

[WIP] Start implementation of permissions command #5831

Closed

Conversation

gxespino
Copy link
Contributor

@gxespino gxespino commented Jul 1, 2017

Hello - This is a WIP in response to #5786.

I'm envisioning a bundle permissions command that will display either a:

  1. Permissions satisfied message
  2. Permissions warning in the case bundle install can proceed, but the bundle_path and/or any child directories are not owned by the executing user.
  3. Permissions error in the case bundle install cannot proceed

bundle permissions will be ran as a part of bundle doctor.

Let me know if I'm headed down the right path. Also, any ideas on how to simulate changing the user of a directory in tests? Can't seem to get specification 2 to work.

Lastly, taking recommendations on the specific message verbiage!

Note: Will fill out PR questionnaire upon completion.

@colby-swandale
Copy link
Member

Thanks for tackling this issue.

Why did you decide to make this a command? The use case i had when i wrote the issue was that users could just run bundle doctor if they were having issues with file permissions, so the bundle permissions command seems a bit redundant.

@gxespino
Copy link
Contributor Author

gxespino commented Jul 1, 2017

This definitely doesn't have to be a separate command... but it could? I wrote it separately in order to isolate the logic and test cases with the intention to eventually call Bundler::CLI::Permissions.new({}).run in bundle doctor.

Also, bundle check seemed to establish a pattern for small utility commands in which it was initially separate but then later also included in bundle doctor #4888

Either way - I definitely considered the redundancy that you brought up and would be happy to move all of this code into bundle doctor and have a one-size-fits-all utility command to display potential warnings and errors. Would we want also want to remove bundle check if that was the case?

@segiddins
Copy link
Member

bundle check exists as a separate command because it is intended to be invoked directly by users to see if their dependencies are all installed -- bundle doctor just so happens to call it. Please move these checks to the bundle doctor command

@gxespino
Copy link
Contributor Author

gxespino commented Jul 1, 2017

Sounds good!

@bundlerbot
Copy link
Collaborator

☔ The latest upstream changes (presumably #5980) made this pull request unmergeable. Please resolve the merge conflicts.

@colby-swandale
Copy link
Member

ping @gxespino are you still working on this?

@colby-swandale
Copy link
Member

I'm closing this for now. If you're still working on this PR feel free to re-open it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants