all: fix setupEmbedCfg functions to set cfg.InitialCluster more safely #18009
Conversation
|
Hi @callthingsoff. Thanks for your PR. I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/ok-to-test |
There was a problem hiding this comment.
Thanks for your PR @callthingsoff. I left a possible improvement for your code ✌️
|
QQ, if you are touching this code. Have you double checked if it has any tests? If not please start from adding tests. |
callthingsoff
force-pushed
the
fix_setupEmbedCfg
branch
from
35653f8
to
e4539e1
Compare
There was a problem hiding this comment.
This is a valid solution, and it works. My only concern is that I feel the readability could be improved. I feel that the original solution was intricate. i.e., I think something like this would work too:
initialCluster := make([]string, len(ics))
for i, ic := range ics {
initialCluster[i] = fmt.Sprintf("%d=%s", i, ic.String())
}
cfg.InitialCluster = strings.Join(initialCluster, ",")Ultimately, this code doesn't need to be highly efficient as it's used in the tests and the etcd-dump-metrics tool. So, I may weigh readability over performance. But readability could also be something subjective 😅. So, I'm just leaving this as food for thought.
Thanks, @callthingsoff.
When the slice is empty, it can panic with "index 1 out of bounds". This patch rewrites these functions to set cfg.InitialCluster more safely. Signed-off-by: Jes Cok <xigua67damn@gmail.com>
callthingsoff
force-pushed
the
fix_setupEmbedCfg
branch
from
e4539e1
to
38528ca
Compare
|
/retest |
| }, | ||
| } | ||
| for i, tt := range tests { | ||
| if got := joinURLs(tt.urls); got != tt.want { |
There was a problem hiding this comment.
joinURLs is already part of the test, we are adding test for test. Not necessary to me.
| cfg.InitialCluster = "" | ||
| for i := range ics { | ||
| cfg.InitialCluster += fmt.Sprintf(",%d=%s", i, ics[i].String()) |
There was a problem hiding this comment.
It seems an unnecessary change,
- The function
setupEmbedCfgis already very simple. - The new function
joinURLsisn't used by multiple users (it has only one user).
|
Thanks, closing this. |
When the slice is empty, it can panic with "index 1 out of bounds". This patch rewrites these functions to set cfg.InitialCluster more safely.
Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.