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

[3.4] etcdserver,pkg: remove temp files in snap dir when etcdserver starting #14246

Merged
merged 1 commit into from
Jul 22, 2022
Merged

[3.4] etcdserver,pkg: remove temp files in snap dir when etcdserver starting #14246

merged 1 commit into from
Jul 22, 2022

Conversation

vivekpatani
Copy link
Contributor

@vivekpatani vivekpatani mentioned this pull request Jul 20, 2022
Closed
25 tasks
@ahrtr ahrtr changed the title etcdserver,pkg: remove temp files in snap dir when etcdserver starting [3.4] etcdserver,pkg: remove temp files in snap dir when etcdserver starting Jul 20, 2022
@vivekpatani
Copy link
Contributor Author

@ahrtr looks like I've missed a few things, I'll have it fixed in sometime.

@vivekpatani vivekpatani force-pushed the release-3.4 branch 2 times, most recently from 8d74e80 to abefeea Compare July 20, 2022 21:47
ahrtr
ahrtr reviewed Jul 21, 2022
View reviewed changes
pkg/fileutil/fileutil.go Outdated Show resolved Hide resolved
ahrtr
ahrtr reviewed Jul 21, 2022
View reviewed changes
pkg/fileutil/fileutil_test.go Outdated Show resolved Hide resolved
pkg/fileutil/fileutil_test.go Outdated Show resolved Hide resolved
pkg/fileutil/fileutil_test.go Outdated Show resolved Hide resolved
pkg/fileutil/fileutil_test.go Outdated Show resolved Hide resolved
@serathius
Copy link
Member

serathius commented Jul 21, 2022 •
edited
Loading

I would be worried about making improvements/patches to cherry-picked commits. When cherry-picking we should do minimal changes to original code so it works with older branch.

We should not improve the code during cherry-pick. Back-porting should be focused on fixing bug and not alter the code from main branch. Back-port should also flow from newest release. If there are any places for improvements, please start from main and release-3.5 branches.

@ahrtr
Copy link
Member

ahrtr commented Jul 21, 2022

I would be worried about making improvements/patches to cherry-picked commits. When cherry-picking we should do minimal changes to original code so it works with older branch.

Actually there are already some changes on the main branch, and they are exactly what I pointed out. The defer os.RemoveAll(tmpdir) isn't correct, we should remove it. Other comments isn't a big deal, personally I would suggest to update it as well, but I will not insist on it.

@ahrtr
Copy link
Member

ahrtr commented Jul 21, 2022

Specifically, the PR 12846 is already out of date in the main branch.

@vivekpatani
etcdserver,pkg: remove temp files in snap dir when etcdserver starting
e4deb09 
- Backporting: etcd-io#12846
- Reference: etcd-io#14232

Signed-off-by: vivekpatani <9080894+vivekpatani@users.noreply.github.com>
@vivekpatani vivekpatani requested a review from ahrtr July 21, 2022 23:52
ahrtr
ahrtr approved these changes Jul 22, 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

Thank you @vivekpatani

@ahrtr
Copy link
Member

ahrtr commented Jul 22, 2022

IT's a minor and accordingly safe change, merging...

@ahrtr ahrtr merged commit aca5cd1 into etcd-io:release-3.4 Jul 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@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.

None yet
3 participants
@vivekpatani @serathius @ahrtr