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

[API] GetConfiguration() sets the configuration for other calls #1347

Closed
ferventcoder opened this Issue Jun 22, 2017 · 7 comments

Comments

Projects
None yet
2 participants
@ferventcoder
Copy link
Member

ferventcoder commented Jun 22, 2017

/// <summary>
/// Gets the configuration. Should be used purely for informational purposes
/// </summary>
/// <returns>The configuration for Chocolatey</returns>
/// <remarks>Only call this once you have registered all container components with Chocolatey</remarks>
public ChocolateyConfiguration GetConfiguration()
doesn't call out possible side effects efficiently enough.

@ferventcoder ferventcoder added this to the 0.10.8 milestone Jun 22, 2017

@ferventcoder ferventcoder self-assigned this Jun 22, 2017

@ferventcoder ferventcoder changed the title [API] Call out warning about using GetConfiguration() side effects (or fix) [API] GetConfiguration() sets the configuration for other calls Aug 29, 2017

@ferventcoder

This comment has been minimized.

Copy link
Member Author

ferventcoder commented Aug 29, 2017

Hopefully the changes made for 0.10.8 fix issues with repeated values making it through.

Just ensure that you never access ChocolateyConfiguration through the container or Config.get_configuration_settings() and make changes directly to it as it has unforeseen side effects. FYI, @mwrock - can you make sure that this is the case in boxstarter? All configuration should be passed in through GetChocolatey.Set() and nowhere else.

Added this to GetChocolatey summary:

/// NOTE: When using the API, this is the only means of accessing the ChocolateyConfiguration without side effects.
/// DO NOT call `Config.get_configuration_settings()` or access the container to pull out the ChocolateyConfiguration.
/// Doing so can set configuration items that are retained on next use.
@batzen

This comment has been minimized.

Copy link

batzen commented Aug 29, 2017

Wouldn't it be better to return a cloned config in the mentioned methods to prevent this issue? Or would that cause more harm?

@ferventcoder

This comment has been minimized.

Copy link
Member Author

ferventcoder commented Aug 29, 2017

@batzen that is exactly what the API does. If those mentioned methods returned a cloned config as well, it would cause more harm than good.

@ferventcoder

This comment has been minimized.

Copy link
Member Author

ferventcoder commented Aug 29, 2017

The one thing I think may still be an issue is that with Boxstarter there may be some nesting, so when one Configuration is returned, that gets used with the nested packages as well instead of finishing the use, then allowing the config to return to the default setting for use with the next command call.

@batzen

This comment has been minimized.

Copy link

batzen commented Aug 29, 2017

I guess every package in boxstarter is nested and that's the problem.

@ferventcoder

This comment has been minimized.

Copy link
Member Author

ferventcoder commented Aug 29, 2017

The problem comes down to a base configuration that is set early on that needs to be maintained across multiple calls. #1296 has more information on the reasoning and issues that were there previously.

@batzen

This comment has been minimized.

Copy link

batzen commented Aug 29, 2017

I guess the only one really knowing what has to be changed in boxstarter is @mwrock.
I currently got no time to get familiar with the boxstarter sources to give educated comments.

ferventcoder added a commit that referenced this issue Aug 29, 2017

(GH-1347) API method comments for Configuration
Add comments on using ChocolateyConfiguration directly. Also provide
warnings against nested calls to certain commands.

ferventcoder added a commit that referenced this issue Aug 29, 2017

(GH-1347) API Fix: GetConfiguration() manipulates config
Previously, `GetChocoaltey.GetConfiguration()` would allow manipulating
the configuration for other calls when using Chocolatey's API. Ensure
that it is purely used for informational purposes and won't affect the
settings that are used when running Chocolatey's API with future calls.

ferventcoder added a commit that referenced this issue Aug 29, 2017

Merge branch 'stable'
* stable:
  (GH-1347) API Fix: GetConfiguration() manipulates config
  (GH-1395) API - dependency log4net >= 2.0.3
  (GH-1396) API Fix: config output is explicitly set
  (GH-1347) API method comments for Configuration
  (maint) API -move GetConfiguration

@ferventcoder ferventcoder added 3 - Done and removed 2 - Working labels Aug 29, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.