Improved handling of connection strings in Configuration Tab #671

Merged
merged 3 commits into from Dec 20, 2013

Conversation

Projects
None yet
3 participants
@CGijbels
Collaborator

CGijbels commented Dec 10, 2013

  • Added additional warning message indicating when the assumption is made that when a provider is not configured, that Glimpse assumes System.Data.SqlClient
  • Added error messages in case something went wrong
  • Added fallback scenario in case the connection string could not be parsed. The fallback is creating a list of key value pairs from the connection string. The same info as the raw value is shown but in table mode
  • It is now possible to obfuscate multiple parts (values of specific keys) in the same connection string
  • It is now possible to add additional keys to obfuscate by setting an AppSetting. It is not part of the Glimpse configuration but this allows for additional flexibility while not impacting the configuration. For instance
    <add 
        key="Glimpse:ConfigurationTab:ConnectionStrings;KeysToObfuscate"
        value="SomeSpecialKey;SomeOtherSpecialKey"/>
@nikmd23

This comment has been minimized.

Show comment
Hide comment
@nikmd23

nikmd23 Dec 18, 2013

Member

@CGijbels any reason you decided to use appSettings rather than the custom <glimpse> element that houses all the other Glimpse settings?

Member

nikmd23 commented Dec 18, 2013

@CGijbels any reason you decided to use appSettings rather than the custom <glimpse> element that houses all the other Glimpse settings?

@CGijbels

This comment has been minimized.

Show comment
Hide comment
@CGijbels

CGijbels Dec 18, 2013

Collaborator

@nikmd23 because from what I've understood from @avanderhoorn and from you as well, is that we try not to touch the current Configuration until v2? I can add it if you want although we need to think about how to store that in the config, since it is quite hierarchical?

Collaborator

CGijbels commented Dec 18, 2013

@nikmd23 because from what I've understood from @avanderhoorn and from you as well, is that we try not to touch the current Configuration until v2? I can add it if you want although we need to think about how to store that in the config, since it is quite hierarchical?

@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn Dec 18, 2013

Member

I've talked with @nikmd23 and because this isn't likely to be something that changes with the v2 config api changes (vs the blacklisting stuff which probably will), its good to go ahead and make the changes.

It might be worth giving some thought to how we will allow tabs/components to have their own arbitrary configuration - as this is the first time that we are really allowing users to configure the system like this.

Member

avanderhoorn commented Dec 18, 2013

I've talked with @nikmd23 and because this isn't likely to be something that changes with the v2 config api changes (vs the blacklisting stuff which probably will), its good to go ahead and make the changes.

It might be worth giving some thought to how we will allow tabs/components to have their own arbitrary configuration - as this is the first time that we are really allowing users to configure the system like this.

@CGijbels

This comment has been minimized.

Show comment
Hide comment
@CGijbels

CGijbels Dec 19, 2013

Collaborator

Ok, I prepared a suggestion for the config approach, but since the release is tomorrow, we won't have enough time to look at it.

Would you mind merging this one in, since it contains fixes as well. I know the appSettings approach will be included, but if we don't mention it explicitly then normally nobody will rely on it, and even if they do, then they will need to change their config with the next release.

What do you think?

Collaborator

CGijbels commented Dec 19, 2013

Ok, I prepared a suggestion for the config approach, but since the release is tomorrow, we won't have enough time to look at it.

Would you mind merging this one in, since it contains fixes as well. I know the appSettings approach will be included, but if we don't mention it explicitly then normally nobody will rely on it, and even if they do, then they will need to change their config with the next release.

What do you think?

@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn Dec 19, 2013

Member

Sounds doable. Lets bring it in and we can sort out the config later on.

On Thu, Dec 19, 2013 at 12:19 PM, Christophe Gijbels <
notifications@github.com> wrote:

Ok, I prepared a suggestion for the config approach, but since the release
is tomorrow, we won't have enough time to look at it.

Would you mind merging this one in, since it contains fixes as well. I
know the appSettings approach will be included, but if we don't mention
it explicitly then normally nobody will rely on it, and even if they do,
then they will need to change their config with the next release.

What do you think?


Reply to this email directly or view it on GitHubhttps://github.com/Glimpse/Glimpse/pull/671#issuecomment-30947603
.

Member

avanderhoorn commented Dec 19, 2013

Sounds doable. Lets bring it in and we can sort out the config later on.

On Thu, Dec 19, 2013 at 12:19 PM, Christophe Gijbels <
notifications@github.com> wrote:

Ok, I prepared a suggestion for the config approach, but since the release
is tomorrow, we won't have enough time to look at it.

Would you mind merging this one in, since it contains fixes as well. I
know the appSettings approach will be included, but if we don't mention
it explicitly then normally nobody will rely on it, and even if they do,
then they will need to change their config with the next release.

What do you think?


Reply to this email directly or view it on GitHubhttps://github.com/Glimpse/Glimpse/pull/671#issuecomment-30947603
.

@ghost ghost assigned CGijbels Dec 20, 2013

avanderhoorn added a commit that referenced this pull request Dec 20, 2013

Merge pull request #671 from Glimpse/no669-configuration-tab-connecti…
…on-strings-parsing

Improved handling of Connectionstrings in Configuration Tab

@avanderhoorn avanderhoorn merged commit b90f92b into master Dec 20, 2013

@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn Dec 20, 2013

Member

p.s. good work with getting this all tested!

Member

avanderhoorn commented Dec 20, 2013

p.s. good work with getting this all tested!

@nikmd23 nikmd23 deleted the no669-configuration-tab-connection-strings-parsing branch Jul 16, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment