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

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

Merged
merged 1 commit into from
Jul 1, 2015

Conversation

bastianjohn
Copy link
Contributor

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
Copy link
Owner

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

@jason-roberts jason-roberts added this to the Version 3.1 milestone Jun 15, 2015
@craig-wagner
Copy link
Contributor

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
Copy link
Owner

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
Copy link
Contributor

@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 added a commit that referenced this pull request Jul 1, 2015
Handled a ConnectionStringName in the BooleanSQLServerProvider class to prevent duplicated connection strings
@jason-roberts jason-roberts merged commit 821326b into jason-roberts:master Jul 1, 2015
jason-roberts added a commit that referenced this pull request Jul 1, 2015
@jason-roberts jason-roberts changed the title Handled a ConnectionStringName in the BooleanSQLServerProvider class to prevent duplicated connection strings Enable BooleanSQLServerProvider connection to be configured via <connectionStrings> .config element in addition to <appSettings> Jul 2, 2015
jason-roberts added a commit that referenced this pull request Jul 2, 2015
jason-roberts added a commit that referenced this pull request Jul 2, 2015
@jason-roberts
Copy link
Owner

@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
Copy link

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 fc3ae2a 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
Copy link
Contributor

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
Copy link
Owner

@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
Copy link
Owner

Created new issue for this #96

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.

4 participants