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.

Sign up for GitHub

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

Jump to bottom

Removing deprecated commands in etcdctl & etcdutl #13809

Merged
merged 1 commit into from
Mar 23, 2022
Merged

Removing deprecated commands in etcdctl & etcdutl #13809

merged 1 commit into from
Mar 23, 2022

Conversation

kkkkun
Copy link
Contributor

@kkkkun kkkkun commented Mar 16, 2022 •
edited
Loading

@kkkkun kkkkun force-pushed the cleanup-deprecated-commands branch from df243f6 to ee11f4b Compare March 16, 2022 12:32
@kkkkun kkkkun mentioned this pull request Mar 16, 2022
Closed
3 tasks
serathius
serathius reviewed Mar 16, 2022
View reviewed changes
etcdctl/README.md Outdated Show resolved Hide resolved
serathius
serathius reviewed Mar 16, 2022
View reviewed changes
etcdctl/README.md Outdated Show resolved Hide resolved
serathius
serathius reviewed Mar 16, 2022
View reviewed changes
tests/e2e/ctl_v3_snapshot_test.go
@@ -136,6 134,7 @@ func ctlV3SnapshotSave(cx ctlCtx, fpath string) error {
}

func getSnapshotStatus(cx ctlCtx, fpath string) (snapshot.Status, error) {
cx.etcdutl = true
Copy link
Member

Choose a reason for hiding this comment

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

Would be good to make etcdutl default for all removed commands and remove withEtcdutl() and ctlCtx.etcdutl. Can be either in the same or separate PR.

Copy link
Contributor Author

@kkkkun kkkkun Mar 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Other fix expect this. I will commit another PR.

Copy link
Member

Choose a reason for hiding this comment

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

sg

@kkkkun kkkkun force-pushed the cleanup-deprecated-commands branch from ee11f4b to c870cbf Compare March 16, 2022 13:09
@codecov-commenter
Copy link

codecov-commenter commented Mar 16, 2022 •
edited
Loading

Codecov Report

Merging #13809 (f82bef2) into main (35c3cea) will increase coverage by 0.07%.
The diff coverage is n/a.

❗ Current head f82bef2 differs from pull request most recent head 3c8fb67. Consider uploading reports for the commit 3c8fb67 to get more accurate results

@@            Coverage Diff             @@
##             main   #13809       /-   ##
==========================================
  Coverage   72.62%   72.69%    0.07%     
==========================================
  Files         467      467              
  Lines       38276    38318       42     
==========================================
  Hits        27797    27856       59     
  Misses       8678     8654      -24     
- Partials     1801     1808        7
Flag Coverage Δ
all 72.69% <ø> ( 0.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
etcdctl/ctlv3/command/snapshot_command.go 70.00% <ø> (-14.06%) ⬇️
etcdutl/etcdutl/snapshot_command.go 80.55% <ø> ( 3.17%) ⬆️
client/pkg/v3/fileutil/purge.go 66.03% <0.00%> (-7.55%) ⬇️
client/v3/experimental/recipes/queue.go 58.62% <0.00%> (-6.90%) ⬇️
client/v3/concurrency/session.go 88.63% <0.00%> (-4.55%) ⬇️
server/etcdserver/cluster_util.go 70.35% <0.00%> (-3.17%) ⬇️
client/v3/concurrency/election.go 79.68% <0.00%> (-2.35%) ⬇️
etcdctl/ctlv3/command/printer.go 42.96% <0.00%> (-1.39%) ⬇️
client/v3/maintenance.go 54.71% <0.00%> (-1.26%) ⬇️
client/v3/client.go 77.63% <0.00%> (-0.96%) ⬇️
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 35c3cea...3c8fb67. Read the comment docs.

serathius
serathius approved these changes Mar 16, 2022
View reviewed changes
Copy link
Member

@serathius serathius left a comment

Choose a reason for hiding this comment

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

cc @ptabor

ahrtr
ahrtr reviewed Mar 16, 2022
View reviewed changes
CHANGELOG/CHANGELOG-3.6.md Outdated
@@ -16,6 16,9 @@ See [code changes](https://github.com/etcd-io/etcd/compare/v3.5.0...v3.6.0).
### Deprecations

- Deprecated [V2 discovery](https://etcd.io/docs/v3.5/dev-internal/discovery_protocol/).
- Removed [etcdctl defrag --data-dir ...](https://github.com/etcd-io/etcd/pull/13793)
- Removed [etcdctl snapshot restore/status & etcdutl save ...](https://github.com/etcd-io/etcd/pull/13809).
Copy link
Member

Choose a reason for hiding this comment

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

Suggest to add three entries here, one for each removal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's make sense. Thanks.

ahrtr
ahrtr approved these changes Mar 16, 2022
View reviewed changes
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 with a minor comment, which I am also OK to keep unchanged.

@kkkkun kkkkun force-pushed the cleanup-deprecated-commands branch from c870cbf to 71146f2 Compare March 18, 2022 03:03
spzala
spzala approved these changes Mar 21, 2022
View reviewed changes
Copy link
Member

@spzala spzala left a comment

Choose a reason for hiding this comment

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

@kkkkun lgtm
I have one comment, if you want to address it later that's fine but if you can address it in this PR that would be good too. Thanks!

etcdctl/README.md Outdated Show resolved Hide resolved
@kkkkun kkkkun force-pushed the cleanup-deprecated-commands branch from 71146f2 to 0196817 Compare March 22, 2022 07:10
@kkkkun kkkkun force-pushed the cleanup-deprecated-commands branch from 0196817 to f82bef2 Compare March 22, 2022 12:25
@kkkkun
Copy link
Contributor Author

kkkkun commented Mar 22, 2022 •
edited
Loading

I tryied twice. semaphoreci — The build failed on Semaphore. has reached limit.

Should we change something to fix this limit ?

Step 1/14 : FROM ubuntu:21.10 toomanyrequests: You have reached your pull rate limit. You may increase the limit by authenticating and upgrading: https://www.docker.com/increase-rate-limit make[1]: *** [build-docker-test] Error 1 make[1]: Leaving directory/home/runner/etcd'
make: *** [ensure-docker-test-image-exists] Error 2
`

@kkkkun kkkkun force-pushed the cleanup-deprecated-commands branch from f82bef2 to 3c8fb67 Compare March 22, 2022 12:33
@spzala
Copy link
Member

spzala commented Mar 23, 2022 •
edited
Loading

@kkkkun thanks for addressing my comment. Yes, we need a permanent fix for semaphoreci error. There is also an open issue to disable it #13448 but it's something we couldn't figure out yet. I am merging this PR as the build passes except semaphoreci error which is not related to changes. Thanks!
P.S. one more comment below. Thanks!

spzala
spzala reviewed Mar 23, 2022
View reviewed changes
CHANGELOG/CHANGELOG-3.6.md Outdated Show resolved Hide resolved
@kkkkun kkkkun force-pushed the cleanup-deprecated-commands branch from 3c8fb67 to 1649c9d Compare March 23, 2022 03:06
@spzala spzala merged commit 3254125 into etcd-io:main Mar 23, 2022
@kkkkun kkkkun mentioned this pull request Mar 24, 2022
Merged
@ahrtr ahrtr mentioned this pull request Apr 13, 2022
Closed
@robert-cronin robert-cronin mentioned this pull request Jun 12, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spzala spzala spzala approved these changes

@serathius serathius serathius approved these changes

@ahrtr ahrtr ahrtr approved these changes

Assignees
No one assigned
Labels
stage/merge-when-tests-green
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kkkkun @codecov-commenter @spzala @serathius @ahrtr