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

Config Server Data dictionary not cleared before Load #185

Merged
merged 2 commits into from
Dec 12, 2019
Merged

Config Server Data dictionary not cleared before Load #185

merged 2 commits into from
Dec 12, 2019

Conversation

dtillman
Copy link
Contributor

@dtillman dtillman commented Dec 2, 2019

[#184] The Config Server's Data dicitonary needs to be cleared before loading it

[#184] The Config Server's Data dicitonary needs to be cleared before loading it
@dnfclas
Copy link

dnfclas commented Dec 2, 2019

CLA assistant check
All CLA requirements met.

@codecov
Copy link

codecov bot commented Dec 2, 2019

Codecov Report

Merging #185 into 2.x will decrease coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##              2.x     #185      +/-   ##
==========================================
- Coverage   61.74%   61.71%   -0.03%     
==========================================
  Files         784      784              
  Lines       22557    22558       +1     
  Branches     4389     4389              
==========================================
- Hits        13928    13922       -6     
- Misses       7438     7452      +14     
+ Partials     1191     1184       -7
Impacted Files Coverage Δ
...figServerBase/ConfigServerConfigurationProvider.cs 73.09% <100%> (+0.07%) ⬆️
...Core/Metrics/Observer/AspNetCoreHostingObserver.cs 76.66% <0%> (-15%) ⬇️
...ndpointBase/Metrics/Observer/CLRRuntimeObserver.cs 90% <0%> (-5.56%) ⬇️
.../Logging/src/DynamicLogger/DynamicConsoleLogger.cs 90.47% <0%> (+4.76%) ⬆️
.../src/DynamicLogger/DynamicConsoleLoggerProvider.cs 90.72% <0%> (+6.18%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c01886f...5ee5668. Read the comment docs.

@@ -264,6 +264,8 @@ internal ConfigEnvironment DoLoad(bool updateDictionary = true)
"Located environment: {name}, {profiles}, {label}, {version}, {state}", env.Name, env.Profiles, env.Label, env.Version, env.State);
if (updateDictionary)
{
Data.Clear();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dtillman I won't add this one here, this could impact requests which are currently reading the dictinoary value. I would add another local temp dictonary and add the new values init and replace with existing one .

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh .. great point .. thanks!
Dave

Copy link

@vivekkjain vivekkjain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! I hope this is not limited to 2.x.

@jkonicki
Copy link
Contributor

jkonicki commented Dec 2, 2019

No worries, @vivekkjain. We always merge the 2.x branch forward to master (3.x branch).

Thanks for opening the issue and the review.

@TimHess TimHess self-requested a review December 4, 2019 19:59
@TimHess TimHess merged commit 1f1279a into 2.x Dec 12, 2019
@TimHess TimHess deleted the Fix184 branch December 12, 2019 21:36
@TimHess TimHess added ReleaseLine/2.x Identified as a feature/fix for the 2.x release line Type/bug Something isn't working Component/Configuration Issues related to Configuration providers labels Sep 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component/Configuration Issues related to Configuration providers ReleaseLine/2.x Identified as a feature/fix for the 2.x release line Type/bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants