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 CLI Parity] add squash to config structure tags to make decoding easier #2044

Merged

Conversation

ashmeenkaur
Copy link
Collaborator

@ashmeenkaur ashmeenkaur commented Jun 21, 2024

Description

Add "squash" to config structure tags to make decoding easier from legacy flagStorage & mountConfig.
This is a temporary change to make transition easier to new CLI and config flags and will be reverted post transition.
Reason to revert this in future - This change doesn't allow having leaf config with conflicting names.

Reference: https://pkg.go.dev/github.com/mitchellh/mapstructure#hdr-Embedded_Structs_and_Squashing
Open issue on mapstructure library which is blocking the change: mitchellh/mapstructure#356

Link to the issue in case of a bug fix.

NA

Testing details

  1. Manual - Manually verified that viper configs and cobra flags are still working as intended
  2. Unit tests - NA
  3. Integration tests - NA

Copy link

codecov bot commented Jun 21, 2024

Codecov Report

Attention: Patch coverage is 0% with 55 lines in your changes missing coverage. Please review.

Project coverage is 71.52%. Comparing base (52b1e10) to head (bac27e3).

Files Patch % Lines
cfg/config.go 0.00% 55 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2044      +/-   ##
==========================================
+ Coverage   71.46%   71.52%   +0.06%     
==========================================
  Files          99       99              
  Lines       10800    10800              
==========================================
+ Hits         7718     7725       +7     
+ Misses       2735     2728       -7     
  Partials      347      347              
Flag Coverage Δ
unittests 71.52% <0.00%> (+0.06%) ⬆️

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

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

@ashmeenkaur ashmeenkaur changed the title add squash to config structure tags to make decoding easier [Config CLI Parity] add squash to config structure tags to make decoding easier Jun 21, 2024
@ashmeenkaur ashmeenkaur marked this pull request as ready for review June 21, 2024 06:21
@ashmeenkaur ashmeenkaur requested a review from a team as a code owner June 21, 2024 06:21
Copy link
Collaborator

@kislaykishore kislaykishore left a comment

Choose a reason for hiding this comment

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

Please verify that the logic works for two levels of nesting. As per this, if a structure is not nested, that's when we can include the embedded struct to the tag value. It doesn't explicitly talk about the case where the struct is nested.

@ashmeenkaur
Copy link
Collaborator Author

s not nested, that's when we can include the embedded struct to the tag value. It doesn't explicitly talk about the case where the struct is nested.

Verified that it is working for nested structs too

Copy link
Collaborator

@raj-prince raj-prince left a comment

Choose a reason for hiding this comment

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

Relying on Kislay's review.

@ashmeenkaur ashmeenkaur merged commit d4451a4 into master Jun 24, 2024
15 checks passed
@ashmeenkaur ashmeenkaur deleted the add_squash_to_config_object_to_make_decoding_easier branch June 24, 2024 04:09
ashmeenkaur added a commit that referenced this pull request Jun 24, 2024
… from legacy flagStorage & mountConfig. (#2044)"

This reverts commit d4451a4.
ashmeenkaur added a commit that referenced this pull request Jun 24, 2024
… from legacy flagStorage & mountConfig. (#2044)"

This reverts commit d4451a4.
ashmeenkaur added a commit that referenced this pull request Jun 24, 2024
… from legacy flagStorage & mountConfig. (#2044)" (#2058)

This reverts commit d4451a4.
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.

3 participants