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

Reject configuration with unknown fields #2981

Merged
merged 2 commits into from
Nov 5, 2024

Conversation

End-rey
Copy link
Contributor

@End-rey End-rey commented Oct 25, 2024

Closes #2663, #1950.

The problems I have encountered:

  • node config: grpc section with list of structures and node section with unlimited number of attributes.
  • .env files are not being processed, because of the work of the viper package. Should I do it? It might be more difficult.

Copy link

codecov bot commented Oct 25, 2024

Codecov Report

Attention: Patch coverage is 86.11111% with 15 lines in your changes missing coverage. Please review.

Project coverage is 23.12%. Comparing base (838126c) to head (652e230).
Report is 14 commits behind head on master.

Files with missing lines Patch % Lines
cmd/neofs-ir/defaults.go 0.00% 7 Missing ⚠️
cmd/internal/configvalidator/validate.go 91.54% 4 Missing and 2 partials ⚠️
cmd/neofs-node/config/config.go 50.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2981      +/-   ##
==========================================
+ Coverage   22.88%   23.12%   +0.23%     
==========================================
  Files         785      788       +3     
  Lines       58806    58912     +106     
==========================================
+ Hits        13457    13622     +165     
+ Misses      44469    44407      -62     
- Partials      880      883       +3     

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

@End-rey End-rey force-pushed the 2663-reject-configuration-with-unknown-fields branch from 3918309 to 69ecefa Compare October 28, 2024 11:56
@End-rey
Copy link
Contributor Author

End-rey commented Oct 28, 2024

There was a problem that the default value of the list was a slice with a length of 0, so I added a check for this. And now my devenv works.
But it seems like automated system tests didn't pass because the ir or node didn't run well. How can I find out what the error was that caused them to fail? Maybe there is a bad configuration?

@End-rey End-rey self-assigned this Oct 28, 2024
Copy link
Member

@roman-khimov roman-khimov left a comment

Choose a reason for hiding this comment

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

cmd/internal/configvalidator/validate.go Outdated Show resolved Hide resolved
cmd/neofs-node/config/config.go Show resolved Hide resolved
cmd/neofs-ir/internal/validate/config.go Show resolved Hide resolved
cmd/neofs-ir/defaults.go Show resolved Hide resolved
cmd/internal/configvalidator/validate.go Outdated Show resolved Hide resolved
cmd/internal/configvalidator/validate.go Outdated Show resolved Hide resolved
cmd/internal/configvalidator/validate.go Show resolved Hide resolved
cmd/neofs-ir/internal/validate/config.go Show resolved Hide resolved
@End-rey
Copy link
Contributor Author

End-rey commented Oct 30, 2024

Can this be solved with https://pkg.go.dev/github.com/spf13/viper#UnmarshalExact?

Partially it can, but there are such fields in the config, for example, node.attribute_* and there can be as many as you like, so the tag ,remain is used, because of which any incorrect fields will fall into this field and then validation will be successful, although in fact there will be an error. I tried this method and it didn't pass my tests.

Signed-off-by: Andrey Butusov <andrey@nspcc.io>
Add validation of ir and node configs by new structs with all possible fields.
For node config add new option `WithValidate`, that is responsible for whether
the config structure needs to be validated. Add tests.

Closes #2663, #1950.

Signed-off-by: Andrey Butusov <andrey@nspcc.io>
@roman-khimov roman-khimov merged commit 2e583e5 into master Nov 5, 2024
19 of 21 checks passed
@roman-khimov roman-khimov deleted the 2663-reject-configuration-with-unknown-fields branch November 5, 2024 06:56
End-rey added a commit to nspcc-dev/neofs-testcases that referenced this pull request Nov 5, 2024
Because of nspcc-dev/neofs-node#2981, now the config is
being validated, and some fields need to be corrected in the tests.

Signed-off-by: Andrey Butusov <andrey@nspcc.io>
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.

Reject configuration with unknown fields
2 participants