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-1059) Fix for setting global variables #1150

Closed
wants to merge 1 commit into from

Conversation

alex3305
Copy link

@alex3305 alex3305 commented Jan 25, 2017

commandExecutionTimeoutSeconds was defined twice in the Chocolatey
configuration. With this pull request, only the primary key is used. This
fixes the incorrect behaviour that choco exhibited when changing this
value through choco config.

Closes #1059 #973.

For testing use the following commands:

PS> choco config set commandExecutionTimeoutSeconds 7200
PS> choco config get commandExecutionTimeoutSeconds

In the old version, the default value of 2700 was displayed. In this change
the value will be set at the given value.

@ferventcoder
Copy link
Member

Thanks! I will review this and look to get it fixed for 0.10.4.

@ferventcoder ferventcoder self-assigned this Jan 25, 2017
@ferventcoder
Copy link
Member

In the interim, can you review the CONTRIBUTING document for how the git commit should be done and make those fixes? Much appreciated.

@alex3305 alex3305 changed the title Fix for issue 1059. [GH-1059]Fix for setting global variables Jan 25, 2017
@alex3305
Copy link
Author

Reworked my issue according to CONTRIBUTING.md. Thanks for the feedback.

@ferventcoder ferventcoder changed the title [GH-1059]Fix for setting global variables (GH-1059) Fix for setting global variables Jan 25, 2017
@ferventcoder
Copy link
Member

The actual git commit itself will need amended - note contributing does mention how to do this. Also, we can be sticklers on that summary - "(GH-###) summary" is how we want to see it. So parentheses, then a space, then the summary. And try to keep that to about 50ish characters.

Copy link
Member

@ferventcoder ferventcoder left a comment

Choose a reason for hiding this comment

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

Going to review this a bit more but it looks pretty good to me!

commandExecutionTimeoutSeconds was defined twice in the Chocolatey
configuration. With this pull request, only the primary key is used. This
fixes the incorrect behaviour that choco exhibited when changing this
value through `choco config`.

Closes chocolatey#1059 chocolatey#973 .
@alex3305
Copy link
Author

Fixed it for you :). At least I hope I got it right now. Thanks for the feedback :)

@alex3305
Copy link
Author

alex3305 commented Feb 16, 2017

I've created a workaround for setting the variable through Powershell's XML interface:

$path = "C:\ProgramData\chocolatey\config\chocolatey.config"
$xml = [xml](Get-Content $path)
$xml.chocolatey.commandExecutionTimeoutSeconds = "2700"
$xml.SelectSingleNode('//chocolatey/config/add[@key="commandExecutionTimeoutSeconds"]').value = "2700"
$xml.SelectSingleNode('//chocolatey/config/add[@key="webRequestTimeoutSeconds"]').value = "30"
$xml.Save($path)

This is an easy way to modify the config file within a scripting environment.

@ferventcoder
Copy link
Member

@alex3305 this looks to have went to the deprecated commandExecutionTimeoutSettings - We haven't used those in a long time, the idea was to upgrade those to the new locations.

I think it's probably okay to remove those now though.

@ferventcoder
Copy link
Member

@alex3305 moved forward on this in a different way. I really appreciate your contributions, hopefully next time we can get a deeper review a little sooner.

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.

"commandExecutionTimeoutSeconds" always reverts to 2700 when deprecated config setting is 0
2 participants