fix: file busy error while deploying through dropgz #1582

vipul-21 · 2022-09-07T20:00:57Z

Reason for Change:
This is a fix for the cyclic dependency in the dropgz deploy subcommand. The deploy command is trying to write a file that is already being executed by some other program.
This change renames the current dest as old one ( deleting the old dest if it exists) and then try to write the new content to the dest.

Requirements:

uses conventional commit messages
includes documentation
adds unit tests

Notes:
Tested the change locally by trying to use the deploy cmd for a file that was running continuously in the background.

dropgz/pkg/embed/payload.go

dropgz/README.md

dropgz/pkg/embed/payload.go

rbtr · 2022-09-07T22:19:33Z

dropgz/pkg/embed/payload.go

+
+	if _, err := os.Stat(dest); err == nil || !os.IsNotExist(err) {
+		old_file_dest := dest + ".old"
+		os.RemoveAll(old_file_dest)


what happens if this remove fails? ignoring the returned error is 👎🏼, if we really don't care we should _ = os.RemoveAll(old_file_dest) it to positively indicate that it doesn't matter.
but if we don't care if this errors...do we even need it?

I think it is better to add an error handler than to silently ignore it. So will be adding it in the next update.

can you explain why or why not we should handle this error?

We should be handling the error in this case, Removeall will return nil if the path does not exists which is good for us for the first time. But there can be other Patherror returned while removing the dest. If we ignore it, then the next part of the code that renames the files will be errored out as well because the old file was not removed in the first place.

why dont we use os.Remove since we are removing single file?

For the first the file won't be present, so os.Remove will error out.

dropgz/pkg/embed/payload.go

rbtr · 2022-09-07T22:34:32Z

@vipul-21 as an aside from the content of this change, PRs are documentation. Please fill in the PR description so that

any teammember will be able to gather the context necessary to review your change, and
anyone in the future (ie you, in 6 months) will be able to look back at this PR and understand the problem you were attempting to fix, the proposed solution, and the context at this point in time.

There should have been a description template prepulated when you opened the PR, so I can only assume that you intentionally deleted that instead of filling it in with useful information.

This same thing applies to your commit history and messages - the commit history on this PR is an unreadable mess, but it should provide documentation which help the reader understand what changes are being made and why.
@timraymond maybe does the best job of this on the whole team, and this recent PR should be a good reference for you 🙂

vipul-21 · 2022-09-08T01:45:54Z

Thank @rbtr for the review. I'll add the description for the PR. Regarding the commit messages, git is tracking everything since start for my PR, even though those comments are already merged. I need to make amends to my fork master branch because the diff only shows the last commit I made, but the PR includes all the commits so far.
Won't happen in the next PR. Thanks for the review, appreciate it :)

rbtr · 2022-09-08T02:10:16Z

The commit history needs to be fixed in this PR. The docs for git rebase and git cherry-pick will help you. Do not update with merge commits. The commit history should contain only the commits related to this change.
In the future, make feature branches instead of committing directly to master (even on your fork), and don't reuse branches that have already been merged for a new set of changes. It will make it easier for you to rebase your changes on top of the latest upstream changes if you keep them well separated.

vipul-21 · 2022-09-08T07:06:16Z

@rbtr There were 22 commits for rebase and had merge conflicts. So i created a separate branch with baseline as master and created the PR: #1586

Closing this one as that would be easier to review. Thanks Evan !

… race condition

rbtr · 2022-09-08T16:58:52Z

dropgz/pkg/embed/payload.go

+		if err = os.RemoveAll(old_file_dest); err != nil {
+			return errors.Wrapf(err, "not able to remove the %s", &old_file_dest)
+		}
+		if err = os.Rename(dest, old_file_dest); err != nil {
+			return errors.Wrapf(err, "not able to rename the %s to %s", dest, old_file_dest)
+		}


I think the os.RemoveAll is unnecessary - os.Rename will remove the file at dest, if it exists.

For the first the file won't be present, so os.Remove will error out.

I didn't say anything about os.Remove

dropgz/pkg/embed/payload.go

…style

dropgz/README.md

dropgz/pkg/embed/payload.go

rbtr requested changes Sep 7, 2022

View reviewed changes

vipul-21 requested a review from rbtr September 8, 2022 02:02

vipul-21 closed this Sep 8, 2022

vipul-21 force-pushed the master branch from 835593e to 5750e0d Compare September 8, 2022 02:45

vipul-21 reopened this Sep 8, 2022

Dropgz deploy cmd - Clearing the dest before writing into it to avoid…

9809e46

… race condition

vipul-21 force-pushed the master branch from 649a6e4 to 9809e46 Compare September 8, 2022 16:47

rbtr requested changes Sep 8, 2022

View reviewed changes

Matching the wrapped comment with the rest of the file

f1db0b0

vipul-21 requested a review from rbtr September 8, 2022 17:46

timraymond requested changes Sep 8, 2022

View reviewed changes

dropgz/pkg/embed/payload.go Outdated Show resolved Hide resolved

dropgz/pkg/embed/payload.go Outdated Show resolved Hide resolved

dropgz/pkg/embed/payload.go Outdated Show resolved Hide resolved

vipul-21 requested a review from timraymond September 8, 2022 19:39

vipul-21 added 2 commits September 8, 2022 12:51

Variable name changes and updating condition to follow line of sight …

c56a792

…style

removing the removall logic

7c134b5

tamilmani1989 requested changes Sep 8, 2022

View reviewed changes

dropgz/README.md Show resolved Hide resolved

dropgz/pkg/embed/payload.go Outdated Show resolved Hide resolved

vipul-21 requested a review from tamilmani1989 September 8, 2022 20:00

tamilmani1989 previously approved these changes Sep 8, 2022

View reviewed changes

Renaming the dest only if it exists

02230bf

vipul-21 dismissed tamilmani1989’s stale review via 02230bf September 10, 2022 07:37

rbtr approved these changes Sep 12, 2022

View reviewed changes

rbtr changed the title ~~fix: cyclic dependency error while deploying through dropgz~~ fix: file busy error while deploying through dropgz Sep 12, 2022

timraymond approved these changes Sep 12, 2022

View reviewed changes

vipul-21 merged commit 08408d7 into Azure:master Sep 12, 2022

fix: file busy error while deploying through dropgz #1582

fix: file busy error while deploying through dropgz #1582

Uh oh!

Conversation

vipul-21 commented Sep 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rbtr commented Sep 7, 2022

Uh oh!

vipul-21 commented Sep 8, 2022

Uh oh!

rbtr commented Sep 8, 2022

Uh oh!

vipul-21 commented Sep 8, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vipul-21 commented Sep 7, 2022 •

edited

Loading