Skip to content

Fix gRPC interface function to merge configs #1164

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

Merged
merged 3 commits into from
Feb 2, 2021

Conversation

silvanocerza
Copy link
Contributor

  • The PR has no duplicates (please search among the Pull Requests
    before creating one)
  • The PR follows
    our contributing guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • UPGRADING.md has been updated with a migration guide (for breaking changes)
  • What kind of change does this PR introduce?

Bug fix.

  • What is the current behavior?

Calling the gRPC interface Merge function to merge existing configs with new ones would ignored empty values.

  • What is the new behavior?

This fix adds the possibility to set empty values with the Merge gRPC function, previously they would have been ignored.

Because of this change I had also to modify the GetValue() function since it would first check if the value was set using the Viper.InConfig() function that wouldn't check for values set with Viper.Set().

No.

  • Other information:

Fixes #1161


See how to contribute

This fix adds the possibility to set empty values with the Merge gRPC
function, previously they would have been ignored.

Because of this change I had also to modify the GetValue() function
since it would first check if the value was set using the
Viper.InConfig() function that wouldn't check for values set with
Viper.Set().
Copy link
Member

@cmaglie cmaglie left a comment

Choose a reason for hiding this comment

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

I've added some suggestions, overall LGTM 👍

@silvanocerza silvanocerza merged commit fa478dd into master Feb 2, 2021
@silvanocerza silvanocerza deleted the scerza/config-merge-fix branch February 2, 2021 16:41
silvanocerza added a commit that referenced this pull request Feb 4, 2021
* Fix gRPC interface function to merge configs

This fix adds the possibility to set empty values with the Merge gRPC
function, previously they would have been ignored.

Because of this change I had also to modify the GetValue() function
since it would first check if the value was set using the
Viper.InConfig() function that wouldn't check for values set with
Viper.Set().

* [skip changelog] Add clearer example to client_example

* [skip changelog] Simplified some code and enhance a test
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.

[gRPC] Neither merge nor setValue can be used to unset a CLI config entry
2 participants