Skip to content

allow textfile to be overwritten on windows#650

Merged
csmarchbanks merged 4 commits intoprometheus:masterfrom
bluppfisk:fix_rename_windows
May 11, 2021
Merged

allow textfile to be overwritten on windows#650
csmarchbanks merged 4 commits intoprometheus:masterfrom
bluppfisk:fix_rename_windows

Conversation

@bluppfisk
Copy link
Copy Markdown
Contributor

This is a prospective fix for issue #649

For POSIX installations, nothing changes. For Windows installations, if python>=3.3 is detected, it will use os.replace, else it will do a double operation of os.remove followed by os.rename, either of which may fail and result in (temporarily or permanently) broken metrics.

This can be refined to catch such errors.

Signed-off-by: Sander Van de Moortel <sander.vandemoortel@gmail.com>
@bluppfisk bluppfisk force-pushed the fix_rename_windows branch from 348eee8 to 0be4d06 Compare May 10, 2021 13:30
Signed-off-by: Sander Van de Moortel <sander.vandemoortel@gmail.com>
@bluppfisk
Copy link
Copy Markdown
Contributor Author

bluppfisk commented May 10, 2021

Python 3.4 test fails because the typing package (used in Tox) was added in v3.5. Unsure how to fix

Copy link
Copy Markdown
Member

@csmarchbanks csmarchbanks left a comment

Choose a reason for hiding this comment

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

Thanks, one comment, but I don't feel too strongly about it.

The test failure appears to be due to a bad release of attrs: https://www.attrs.org/en/stable/changelog.html, but that release has been yanked so i am not sure why it is still being chosen in the tests. I will dig in a bit further when I have a moment.

Signed-off-by: Sander Van de Moortel <sander.vandemoortel@gmail.com>
@bluppfisk bluppfisk force-pushed the fix_rename_windows branch from 2f1bc8d to 1089530 Compare May 10, 2021 20:40
Copy link
Copy Markdown
Member

@csmarchbanks csmarchbanks left a comment

Choose a reason for hiding this comment

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

Thanks for the update! Another comment, but otherwise this looks good to me. I will likely merge without the 3.4 test passing as the failure is clearly unrelated.

Signed-off-by: Sander Van de Moortel <sander.vandemoortel@gmail.com>
Copy link
Copy Markdown
Member

@csmarchbanks csmarchbanks left a comment

Choose a reason for hiding this comment

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

👍 Thanks!

@csmarchbanks csmarchbanks merged commit d8bc027 into prometheus:master May 11, 2021
@bluppfisk bluppfisk deleted the fix_rename_windows branch May 12, 2021 07:03
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.

2 participants