TrueCloudLab/frostfs-sdk-go
32
0
Fork 21
Code Issues 32 Pull requests 3 Releases Activity Actions

netmap: Allow empty values for unknown parameters in network config #235

Merged
fyrchik merged 1 commit from achuprov/frostfs-sdk-go:feat/unknown_network_info_parameters into master 2024-07-10 06:10:58 +00:00
Conversation 0 Commits 1 Files changed 1 -5
1 changed files with 0 additions and 5 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

5
netmap/network_info.go
View file

@ -49,11 +49,6 @@ func (x *NetworkInfo) readFromV2(m netmap.NetworkInfo, checkFieldPresence bool)
mNames[name] = struct{}{}
switch name {
default:
if len(prm.GetValue()) == 0 {
aarifullin marked this conversation as resolved
aarifullin commented 2024-07-09 11:00:53 +00:00
Review
Copy link

We are fixing the issue with MaxECDataCount but allows to other params be empty when they must be non-empty

Didn't you consider the solution that @dkirillov suggested?

and explicitly check which must not contain nil value.

We are fixing the issue with `MaxECDataCount` but allows to other params be empty when they must be non-empty Didn't you consider the solution that @dkirillov suggested? > and explicitly check which must not contain nil value.
achuprov commented 2024-07-09 11:22:26 +00:00
Review
Copy link

I assume that unknown parameters can be anything, known ones must not be nil. @dkirillov @aarifullin, did you mean a configuration of non-nil params from the application?

I assume that unknown parameters can be anything, known ones must not be nil. @dkirillov @aarifullin, did you mean a configuration of non-nil params from the application?
dkirillov commented 2024-07-09 11:42:18 +00:00
Review
Copy link

If now there are not parameters (which isn't mentioned here) that must not be nil then solution is ok.
Currently devenv sets SystemDNS config parameter. Is this parameter one that must not be empty? Is so then we need handle this param. Maybe there are some other parameters that require special treatment.

The #232 is more about allow clients don't update SDK when we add new nil parameter. The solution that I suggested just one of many.

If now there are not parameters (which isn't mentioned here) that must not be nil then solution is ok. Currently devenv sets `SystemDNS` config parameter. Is this parameter one that **must** not be empty? Is so then we need handle this param. Maybe there are some other parameters that require special treatment. The #232 is more about allow clients don't update SDK when we add new nil parameter. The solution that I suggested just one of many.
aarifullin commented 2024-07-09 13:35:49 +00:00
Review
Copy link

Is this parameter one that must not be empty?

Good question. To be honest - I don't know.
For now, I suppose, to keep backward compatibility it's simpler and more correct to ignore values (like in this PR) than to emplace a new parameter name to one of this switch's cases adding some handler.
OK. I am going to resolve this conv

> Is this parameter one that must not be empty? Good question. To be honest - I don't know. For now, I suppose, to keep backward compatibility it's simpler and more correct to ignore values (like in this PR) than to emplace a new parameter name to one of this switch's cases adding some handler. OK. I am going to resolve this conv
err = fmt.Errorf("empty attribute value %s", name)
return true
}
case
configAuditFee,
configStoragePrice,