Skip to content

Conversation

@svrooij
Copy link
Contributor

@svrooij svrooij commented Jul 31, 2023

Proposed change

  • Support for User Secrets by not overriding it.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

As discussed on discourt: https://discord.com/channels/686787269780176998/690899481046024233/1135518233458393108

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the [development checklist][dev-checklist]
  • The code compiles without warnings (code quality chek)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

- Support for User Secrets by not overriding it.
@svrooij
Copy link
Contributor Author

svrooij commented Aug 1, 2023

After some thought I think the original code already supported user secrets, they were just overridden. The last config provider takes precedence. the CreateDefaultBuilder sets:

  • Environment
  • commandline arguments
  • appsettings.json and appsettings.{environment}.json
  • User secrets (if set to development)
  • Environment variables
  • commandline arguments

Then the UseNetDaemonAppSettings adds the following (which thus override the previous set):

  • appsettings.json and appsettings.{env}.json
  • environment
  • yaml

This means that if you would remove the token from the app settings it would not get overriden. Either way, no need to set the configuration providers twise.

Copy link
Collaborator

@helto4real helto4real 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 but I am not sure if environment is still working without the AddEnvironmentVariables() can you make sure that is not broken? As I understand the PR description it will still work right?

@svrooij
Copy link
Contributor Author

svrooij commented Aug 1, 2023

The failing test is strange, they all succeed locally, I explicitly checked.

@codecov
Copy link

codecov bot commented Aug 1, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.12% ⚠️

Comparison is base (994a307) 80.92% compared to head (4e51621) 80.81%.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #898      +/-   ##
==========================================
- Coverage   80.92%   80.81%   -0.12%     
==========================================
  Files         173      173              
  Lines        4320     4326       +6     
  Branches      434      435       +1     
==========================================
  Hits         3496     3496              
- Misses        644      650       +6     
  Partials      180      180              
Flag Coverage Δ
unittests 80.81% <0.00%> (-0.12%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
...mmon/Extensions/IConfigurationBuilderExtensions.cs 72.50% <0.00%> (-21.05%) ⬇️
...Runtime/Common/Extensions/HostBuilderExtensions.cs 37.50% <0.00%> (+3.21%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@svrooij
Copy link
Contributor Author

svrooij commented Aug 1, 2023

@helto4real do you know how it’s possible for the coverage to go down if I removed code? That means the old code was not tested I guess? Want me to add tests for the yams settings part?

@helto4real
Copy link
Collaborator

The failing test is strange, they all succeed locally, I explicitly checked.

No it is ok! Sometimes it does this. The code was never tested in the first place and I am not sure how you would test this efficiently.

Copy link
Collaborator

@helto4real helto4real left a comment

Choose a reason for hiding this comment

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

Ok thanks for clarification and the PR!

@helto4real helto4real merged commit a89c03e into net-daemon:dev Aug 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants