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

achuprov · 2024-07-09T09:20:17Z

achuprov commented

2024-07-09 09:20:17 +00:00

Close #232
Signed-off-by: Alexander Chuprov a.chuprov@yadro.com

Close #232 Signed-off-by: Alexander Chuprov <a.chuprov@yadro.com>

achuprov added 1 commit 2024-07-09 09:20:24 +00:00

[#232 ] netmap: Allow empty values for unknown parameters in network config

DCO / DCO (pull_request) Successful in 6m21s

Details

Tests and linters / Tests (1.21) (pull_request) Successful in 6m13s

Details

Tests and linters / Tests (1.20) (pull_request) Successful in 6m18s

Details

Tests and linters / Lint (pull_request) Successful in 8m7s

Details

51cefd4908

Signed-off-by: Alexander Chuprov <a.chuprov@yadro.com>

achuprov changed title from ~~[#232] netmap: Allow empty values for unknown parameters in network config~~ to netmap: Allow empty values for unknown parameters in network config

2024-07-09 09:21:43 +00:00

achuprov requested review from storage-core-committers 2024-07-09 09:58:31 +00:00

achuprov requested review from storage-core-developers 2024-07-09 09:58:32 +00:00

achuprov requested review from storage-services-committers 2024-07-09 09:58:41 +00:00

achuprov requested review from storage-services-developers 2024-07-09 09:58:41 +00:00

aarifullin reviewed 2024-07-09 11:00:53 +00:00

netmap/network_info.go

					
				@ -50,4 +50,2 @@

						switch name {

						default:

							if len(prm.GetValue()) == 0 {

aarifullin commented

2024-07-09 11:00:53 +00:00

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

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

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

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

aarifullin marked this conversation as resolved

fyrchik approved these changes 2024-07-09 12:43:46 +00:00

aarifullin approved these changes 2024-07-09 13:36:06 +00:00

dkirillov approved these changes 2024-07-09 13:36:55 +00:00

fyrchik merged commit 51cefd4908 into master

2024-07-10 06:10:58 +00:00

fyrchik added the

internal

label 2024-07-10 06:55:58 +00:00

Sign in to join this conversation.

No reviewers