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

fix: fix incompatible type of priv_validator_raddrs #767

Merged
merged 1 commit into from
Mar 15, 2024

Conversation

170210
Copy link
Contributor

@170210 170210 commented Mar 12, 2024

Description

This PR fix incompatible type of priv_validator_raddrs in TOML
The PrivValidatorRemoteAddresses was defined as string array, while the priv_validator_raddrs was of a string type rather than a slice type, which creates a mismatch with the structure of the configuration after parsing the toml file.
Related #692 & #707

Signed-off-by: 170210 <j170210@icloud.com>
@170210 170210 self-assigned this Mar 12, 2024
@170210 170210 added the C: enhancement Classification: New feature or its request, or improvement in maintainability of code label Mar 12, 2024
Copy link

codecov bot commented Mar 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.54%. Comparing base (fc54d22) to head (4ca0270).
Report is 3 commits behind head on main.

❗ Current head 4ca0270 differs from pull request most recent head 01814c0. Consider uploading reports for the commit 01814c0 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #767      +/-   ##
==========================================
- Coverage   66.58%   66.54%   -0.04%     
==========================================
  Files         285      285              
  Lines       37919    37914       -5     
==========================================
- Hits        25248    25231      -17     
- Misses      10871    10881      +10     
- Partials     1800     1802       +2     
Files Coverage Δ
config/config.go 79.36% <ø> (ø)
config/toml.go 74.19% <ø> (ø)
privval/internal/ip_filter.go 68.00% <ø> (-2.38%) ⬇️
privval/internal/null_object_filter.go 100.00% <ø> (ø)

... and 11 files with indirect coverage changes

@ulbqb
Copy link
Member

ulbqb commented Mar 14, 2024

Is it ok not to change parsing process although you change toml format?

func TestIpFilterStringShouldReturnsIP(t *testing.T) {
expected := []string{"127.0.0.1", "192.168.1.10"}
assert.Equal(t, strings.Join(expected, ","), NewIpFilter(expected, nil).String())
}

func (f *IpFilter) String() string {
return strings.Join(f.allowList, ",")
}

And old processes are left.

@170210
Copy link
Contributor Author

170210 commented Mar 14, 2024

Is it ok not to change parsing process although you change toml format?

func (f *IpFilter) String() string {
return strings.Join(f.allowList, ",")
}

I checked currently main branch and #707, this function hasn't been used yet (only used in test), so I think we just need to delete it.

@ulbqb
Copy link
Member

ulbqb commented Mar 14, 2024

Is it ok not to change parsing process although you change toml format?

@170210
Copy link
Contributor Author

170210 commented Mar 14, 2024

Is it ok not to change parsing process although you change toml format?

@ulbqb I think it is ok because after parsing with viper, it is read in as an array.

PrivValidatorRemoteAddresses []string `mapstructure:"priv_validator_raddrs"`

ulbqb
ulbqb previously approved these changes Mar 14, 2024
@ulbqb
Copy link
Member

ulbqb commented Mar 14, 2024

I checked currently main branch and #707, this function hasn't been used yet (only used in test), so I think we just need to delete it.

Sorry, this is for Stringer interface so needed.

@170210 170210 merged commit df661e9 into Finschia:main Mar 15, 2024
43 checks passed
@170210 170210 deleted the fix/toml branch March 15, 2024 05:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: enhancement Classification: New feature or its request, or improvement in maintainability of code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants