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

etcdctl: fix move-leader for multiple endpoints #14445

Merged
merged 1 commit into from
Sep 9, 2022

Conversation

tjungblu
Copy link
Contributor

@tjungblu tjungblu commented Sep 9, 2022

Due to a duplicate call of clientConfigFromCmd, the move-leader command would fail with "conflicting environment variable is shadowed by corresponding command-line flag". Also in scenarios where no command-line flag was supplied.

Signed-off-by: Thomas Jungblut tjungblu@redhat.com


main pick of #14434 and #14441
thanks @ahrtr for triple checking me :)

@@ -88,13 +101,12 @@ func testCtlV3MoveLeader(t *testing.T, cfg e2e.EtcdProcessClusterConfig) {
}
}

os.Setenv("ETCDCTL_API", "3")
defer os.Unsetenv("ETCDCTL_API")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was spouting a warning, so I thought I'd clean this up. Let me know if I should leave it.

Copy link
Member

Choose a reason for hiding this comment

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

ETCDCTL_API is 3 by default, why the case need this? It should be legacy "issue". So it's Ok to remove it.

Due to a duplicate call of clientConfigFromCmd, the move-leader command
would fail with "conflicting environment variable is shadowed by corresponding command-line flag".
Also in scenarios where no command-line flag was supplied.

Signed-off-by: Thomas Jungblut <tjungblu@redhat.com>
@ahrtr
Copy link
Member

ahrtr commented Sep 9, 2022

FYI. #14443

I realized that the SetPflagsFromEnv is a little strange, probably we may need to refactor it a little bit later.

Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

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

LGTM

Thank you @tjungblu

@serathius serathius merged commit 92d8d7f into etcd-io:main Sep 9, 2022
@tjungblu tjungblu deleted the movelead_main branch September 9, 2022 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants