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

Fix rmrf and drop version to 2.9.0 #531

Merged
merged 9 commits into from
Sep 5, 2019
Merged

Conversation

damccorm
Copy link

@damccorm damccorm commented May 1, 2019

Current published version is 2.8.0. Probably not going to major version at this time.

@damccorm damccorm requested a review from ericsciple May 1, 2019 21:32
@stephenmichaelf
Copy link
Member

Can we add more details to this? What are we fixing? What's the new implementation? How is it different? Good to have for historical context.

@stephenmichaelf
Copy link
Member

Are there tests that cover this? Do we need more?

@damccorm
Copy link
Author

damccorm commented May 2, 2019

Sure, for context we're trying to solve the issue here. To summarize, nodes unlinkSync doesn't always actually delete a file, it just decrements its ref-count (which eventually has the effect of deleting it). This means that if the file is open elsewhere (often with antivirus software its being scanned) it doesn't actually get immediately deleted. This tries to address that.

I think existing coverage does a pretty good job with this, not really sure if there's a good way to test that exact behavior. Since we're not using unlinkSync, that edge case should just be avoided entirely anyways.

node/task.ts Outdated Show resolved Hide resolved
@stephenmichaelf
Copy link
Member

Looks good, let's have Eric review before merging.

node/task.ts Outdated Show resolved Hide resolved
node/task.ts Outdated Show resolved Hide resolved
node/task.ts Outdated
// program (e.g. antivirus), it won't be deleted. To address this, we shell out the work to rd/del.
try {
if (fs.statSync(inputPath).isDirectory()) {
debug('removing directory');
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a reason to not put the path you are trying to delete in the debug message?

Copy link
Author

Choose a reason for hiding this comment

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

Nope, will update

Copy link
Contributor

@jtpetty jtpetty left a comment

Choose a reason for hiding this comment

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

LGTM

@damccorm
Copy link
Author

damccorm commented Sep 5, 2019

I'm intentionally not updating the version in package.json. We should probably release all the changes going in and need to follow up in the release notes when we do.

@damccorm damccorm merged commit 68aab72 into master Sep 5, 2019
lkillgore pushed a commit to microsoft/azure-pipelines-tasks that referenced this pull request Feb 19, 2020
(microsoft/azure-pipelines-task-lib#531)
Moved 'mark success' to the end to fix minor issue of the logs saying that the build was successful but then failing
lkillgore pushed a commit to microsoft/azure-pipelines-tasks that referenced this pull request Feb 19, 2020
(microsoft/azure-pipelines-task-lib#531)
Moved 'mark success' to the end to fix minor issue of the logs saying that the build was successful but then failing
lkillgore added a commit to microsoft/azure-pipelines-tasks that referenced this pull request Feb 19, 2020
(microsoft/azure-pipelines-task-lib#531)
Moved 'mark success' to the end to fix minor issue of the logs saying that the build was successful but then failing
fullstackinfo pushed a commit to fullstackinfo/azure-pipelines-task-lib that referenced this pull request Aug 17, 2024
* Fix rmrf and drop version to 2.9.0

* Handle case where it already doesn't exist

* Remove extra brace

* Use old linux behavior

* Consistent logging

* Don't mix implementations

* Add explanatory comment

* Better debugging
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants