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

(GH-274) Allow unattended installs as non-admin user #273

Conversation

smspillaz
Copy link
Contributor

Previously, if a non-admin user attempted to install chocolatey,
a message indicating that non-admin installs were for advanced
users only would appear and block the installation.

This runs contrary to chocolatey policy, so just pass -y during
the unpackself command. The installation should never block.

This closes #274

@ferventcoder
Copy link
Member

Howdy - have you had a chance to review CONTRIBUTING.md?

@ferventcoder
Copy link
Member

What version of choco were you running before? We've removed the admin requirement for a bunch of commands, including unpackself.

@smspillaz
Copy link
Contributor Author

@ferventcoder Hey, I had a brief look - my understanding was that PRs were fine for small changes. Was there anything else I needed to do?

As for which version I'm running - I'm running 0.9.9.5.

I think the main problem here isn't so much the admin requirement as much as the interactive prompt that the command throws up. It blocks fully automated installations, which is important for a continuous integration script I'm working on.

Thanks for taking a look at this so quickly by the way :)

@ferventcoder
Copy link
Member

Then I think it should pass -y by default, don't you?

@ferventcoder
Copy link
Member

I didn't think unpackself threw up any prompts (this is where I could be wrong).

@ferventcoder
Copy link
Member

Hey, I had a brief look - my understanding was that PRs were fine for small changes. Was there anything else I needed to do?

Yes, there is an extensive section on the git commit message. We need to be sticklers there because it makes searching later down the line much easier if commits are formatted correctly with the correct tags on them.

@smspillaz
Copy link
Contributor Author

@ferventcoder Indeed, unpackself prompts about lack of admin access.

Potentially we could pass -y by default.

My understanding is that -y also auto-accepts a lot of other stuff too (simple code search here), which is why I added the environment variable (off by default so that any potential damage is limited.

I'll update the commit message - thanks for the pointer :)

@ferventcoder
Copy link
Member

@smspillaz in the case of unpackself, there is no reason not to have it -y

@ferventcoder
Copy link
Member

Apparently I set that. But it needs to run silently, otherwise it breaks choco conventions, that is why it should go with -y every time.

$chocoNew = Join-Path $thisScriptFolder 'chocolateyInstall\choco.exe'
if ($debugMode) {
& $chocoNew unpackself -fdv
& $chocoNew unpackself -fdv $allowGlobalConfirmation
Copy link
Member

Choose a reason for hiding this comment

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

This makes me think we should set $debugMode as '-dv' and only have one command there instead of two. But also that we should always use -y. If the script stops for any reason, it is breaking Chocolatey conventions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool. Shall I just add -y everywhere then?

@smspillaz smspillaz changed the title nuget: Allow unattended installs as non-admin user (GH-274) Allow unattended installs as non-admin user May 7, 2015
@smspillaz
Copy link
Contributor Author

Filed issue #274 for the commit message.

@ferventcoder
Copy link
Member

Sweet, let me know when you update the commit and repush it, plus the fixes I requested. :)

@ferventcoder
Copy link
Member

I love your first commit message - https://github.com/chocolatey/choco/pull/273/commits - well formed but needs to remove the word nuget: in the summary and (GH-274) prepended. And the second commit should be squashed into the first commit.

I might be getting ahead of you here, you are probably already on making the changes. :)

@smspillaz smspillaz force-pushed the unattended-install-as-non-admin-user branch from 4a73ef3 to 5ab9c05 Compare May 7, 2015 13:16
@smspillaz
Copy link
Contributor Author

@ferventcoder Cool. I've squashed down to two commits (one for the change, one for the cleanup recommended) ...... aaaannd CI just failed.

Do you want those two in separate commits or just one?

@smspillaz
Copy link
Contributor Author

hahaha, it was in the nuget subdirectory :P

@ferventcoder
Copy link
Member

Haha, I guess that is true, the nuget package. :)

@@ -314,9 +310,9 @@ param(
Copy-Item $chocoExe $chocoExeDest -force

if ($debugMode) {
& $chocoExeDest unpackself -fdv
& $chocoExeDest unpackself -fdvy
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh weird, I missed this. Hang on.

@ferventcoder
Copy link
Member

Nah, those two things are separate ideas. I don't believe in squashing separate distinct things into one commit.

@smspillaz smspillaz force-pushed the unattended-install-as-non-admin-user branch from 5ab9c05 to d7894b2 Compare May 7, 2015 13:21
@smspillaz
Copy link
Contributor Author

Oh, I need to update the commit message for the first commit. Hang tight.

smspillaz added 2 commits May 7, 2015 21:23
Previously, if a non-admin user attempted to install chocolatey,
a message indicating that non-admin installs were for advanced
users only would appear and block the installation.

This runs contrary to chocolatey policy, so just pass -y during
the unpackself command. The installation should never block.
Previously we had an if condition on debugMode, but it is more
concise to just express it as a string appended to the list
of options passed to choco.
@smspillaz smspillaz force-pushed the unattended-install-as-non-admin-user branch from d7894b2 to 46d01f6 Compare May 7, 2015 13:24
@smspillaz
Copy link
Contributor Author

(you can cancel all the backed up builds on appveyor by the way, there's probably quite a few O.o)

@ferventcoder
Copy link
Member

Yeah, I better do that. :)

@smspillaz
Copy link
Contributor Author

I didn't see debugMode used anywhere else, no

@smspillaz
Copy link
Contributor Author

@ferventcoder Looks like its all passing (travis-ci is broken by a bad mono package is seems).

@ferventcoder
Copy link
Member

@smspillaz travis-ci has issues sometimes.

@smspillaz
Copy link
Contributor Author

@ferventcoder What's the timeline for getting these changes merged in? I'm waiting to start work on one of my own projects that will use choco under the hood.

@ferventcoder
Copy link
Member

This is going into 0.9.9.6, which will be releasing pretty soon.

@ferventcoder
Copy link
Member

Merged into stable at ae68a69 to be released with 0.9.9.6.

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.

Unattended installation of Chocolatey without admin rights aren't possible.
2 participants