(#3201 #3225) Re-instate setting of config properties #3223
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@AdmiringWorm happy for you to review this PR, but I am now thinking that we need a separate issue to cover the changes in this PR, rather than tagging it against the original issue. Since this is a problem introduced in 2.0.0, we should cover the change in a separate issue in 2.1.0. |
gep13
force-pushed
the
cache-location-fix
branch
from
f6d797e
to
318ee93
Compare
gep13
changed the title
This was
linked to
issues
Jun 23, 2023
gep13
force-pushed
the
cache-location-fix
branch
from
318ee93
to
5e5f804
Compare
|
@AdmiringWorm I believe that this is now ready for review. |
Previously, in this commit: chocolatey@da19356#diff-cb6a0471e41268b22a928bd57a59d51b70b7024e9beb30e89a330e193a089eba The usage of the top level CacheLocation and CommandExecutionTimeoutSeconds values had been removed, since these top level properties within the chocolatey.config had been replaced with values contained within the config section of the chocolatey.config file. However, the changes in that commit were too aggressive, and removed the call to the set_config_item (which has subsequently been renamed to SetConfigItem). The method call does the work of taking any value that is defined in the chocolatey.config for a given property name, and adding it to the ChocolateyConfiguration instance. When this method call was removed, it stopped setting the property value on the instance, and as a result, values that had been configured in the chocolatey.config file were ignored. They were still in play when the configuration was passed in via a command line option, but not when defining them in the chocolatey.config file. The removal of this call to the set_config_item method also explains why it was necessary to apply this bug fix in Chocolatey GUI: chocolatey/ChocolateyGUI#1003 The method call also had the effect of setting the description on the configuration value, which would have meant that Chocolatey GUI wouldn't have thrown a null reference exception. The change in Chocolatey GUI is still valid though, as there are times when a config value can have a missing description, so it makes sense to leave that fix in place.
corbob
force-pushed
the
cache-location-fix
branch
from
5e5f804
to
2c093a0
Compare
| $cacheArg = "--cachedir='$cacheDir'" | ||
| } | ||
| } | ||
| $Output = Invoke-Choco install test-environment --version 1.0.0 $cacheArg |
There was a problem hiding this comment.
@corbob just to note in case you have a need for it.
I have pushed another version of this package to the test packages repository.
It uses a non-normalized version of 0.9 (needed it for my own tests), and it also outputs environment variables during before modify, and uninstalls as well.
corbob
force-pushed
the
cache-location-fix
branch
2 times, most recently
from
770e841
to
b95592c
Compare
Make use of the test-environment package that already exists to get the environment variables set by Chocolatey and ensure that key variables are set as they should be.
corbob
force-pushed
the
cache-location-fix
branch
from
3365433
to
3dc4416
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description Of Changes
Previously, in this commit:
da19356#diff-cb6a0471e41268b22a928bd57a59d51b70b7024e9beb30e89a330e193a089eba
The usage of the top level CacheLocation and
CommandExecutionTimeoutSeconds values had been removed, since these top
level properties within the chocolatey.config had been replaced with
values contained within the config section of the chocolatey.config
file.
However, the changes in that commit were too aggressive, and removed
the call to the set_config_item (which has subsequently been renamed to
SetConfigItem). The method call does the work of taking any value that
is defined in the chocolatey.config for a given property name, and
adding it to the ChocolateyConfiguration instance. When this method
call was removed, it stopped setting the property value on the
instance, and as a result, values that had been configured in the
chocolatey.config file were ignored. They were still in play when the
configuration was passed in via a command line option, but not when
defining them in the chocolatey.config file.
The removal of this call to the set_config_item method also explains
why it was necessary to apply this bug fix in Chocolatey GUI:
chocolatey/ChocolateyGUI#1003
The method call also had the effect of setting the description on the
configuration value, which would have meant that Chocolatey GUI
wouldn't have thrown a null reference exception. The change in
Chocolatey GUI is still valid though, as there are times when a config
value can have a missing description, so it makes sense to leave that
fix in place.
Motivation and Context
Fix a problem that was introduced when usage of the deprecated properties was removed.
Testing
--cache-location c:\choco-cacheoptionchoco config set --name=cachelocation --value c:\choco-cache2These steps are working on the assumption that setting the cache-location value to "something" then results in the TEMP environment variable being set with this value, which is then used within the innosetup package to determine where the log file is created. This previously would not have worked and the log file would never have been created in the choco-cache folder.
Operating Systems Testing
Change Types Made
Change Checklist
Related Issue
Fixes #3201
Fixes #3225