jason-roberts / FeatureToggle

Simple, reliable feature toggles in .NET
http://dontcodetired.com/blog/?tag=/featuretoggle
Apache License 2.0
689 stars 111 forks source link

Enable BooleanSQLServerProvider connection to be configured via <connectionStrings> .config element in addition to <appSettings> #88

Closed bastianjohn closed 9 years ago

bastianjohn commented 9 years ago

Hi, the BooleanSqlServerProvider now checks if there is either a ConnectionString or a ConnectionStringName given in the web.config. If it is the new ConnectionStringName, it uses the connection from the ConnectionString Area in the web.config with the given name. Would be nice to be updated in the next version :) Thanks

jason-roberts commented 9 years ago

Good idea @bastianjohn :) - is it possible to add test(s) for this change?

craig-wagner commented 9 years ago

I was just about to make this change myself but I see someone beat me to it. One comment on the change though, the error message given if the connection string name isn't found or is empty specifically references "the web.config." That could be confusing to people who are using this library in a non-web application. Keeping it more consistent with the first error would probably be better. May I suggest:

The key '{0}' was not found in ConnectionStrings or the ConnectionString property is empty.

Having said that, any idea when 3.1 might be released? I really need this feature largely due to the way we transform our config files during deployment. Having multiple copies of the same connection string makes for a lot of deployment pain.

jason-roberts commented 9 years ago

Hi @craig-wagner - good idea, not sure on release date for 3.1 at the moment, I might look at doing a smaller .1 release with less stuff in it in order to get this in?

craig-wagner commented 9 years ago

@jason-roberts Yeah, if you could do a smaller release to get this in it would be a great help. I could do my own build of the assemblies but then I have to swap that out for the NuGet package later which is also a pain. Thanks for considering it.

jason-roberts commented 9 years ago

@craig-wagner @bastianjohn just released 3.1 to NuGet with these changes in, still being indexed by NuGet so might not appear in search results yet

pascalberger commented 9 years ago

Not sure if this is finally implemented like it was intended by this PR. With the changes in this PR it would have been possible to define the name of a collection string (instead of the full connection string) for each feature toggle. With the changes in fc3ae2a79788368b78285527db78499ec30c0b39 it seems and the example on http://jason-roberts.github.io/FeatureToggle.Docs/pages/usage.html it seems like this no longer is possible, but instead connection strings for feature toggles can be defined in the connectionStrings section.

It would be much more helpful with the initial implementation, so that multiple feature toggles can share a single connection string entry.

craig-wagner commented 9 years ago

I'm afraid I have to agree with @pascalberger, this didn't get implemented in a way that really helps me much. I still need a copy of my primary database connection string for each toggle, plus the one I need for the application itself.

What I was hoping to see was something similar to the original PR code, which would allow me to do either:

<appSettings>    
    <add key="FeatureToggle.MyFeatureToggle.ConnectionString" value="[some connection string]" />
</appSettings>

or

<connectionStrings>
    <add name="primary" connectionString="..." />
</connectionStrings>

<appSettings>
    <add key="FeatureToggle.MyFeatureToggle.ConnectionString" value="primary" />
</appSettings>

Let me know if that doesn't fit your vision for the product. If it does I'll take a crack at a new PR over the weekend (unless someone beats me to it). If not I'll probably start with your code and create a private version for our shop.

jason-roberts commented 9 years ago

@pascalberger @craig-wagner Hi, I understand better now, there is a definite advantage to specifying a single entry only once and then referencing it by name from something like:

<connectionStrings>
    <add name="primary" connectionString="connection-string" />
</connectionStrings>

<appSettings>
    <add key="FeatureToggle.MyFeatureToggle.ConnectionStringName" value="primary" />
</appSettings>

Notice here the appsettings key ends with .ConnectionStringName - because 3.1 has been released we'll need to maintain backwards compatibility or move to version 4 (for semantic versioning reasons) which I don't want to do yet; whilst this makes the implementation a little more complex it respects proper versioning. Happy for you @craig-wagner to create a PR, if you do so could you also add and update tests in src/Tests/FeatureToggle.Integration.Tests/BooleanSqlServerProviderShould.cs - that would be awesome - thanks :)

jason-roberts commented 9 years ago

Created new issue for this #96