10000 allow textfile to be overwritten on windows by bluppfisk · Pull Request #650 · prometheus/client_python · GitHub
[go: up one dir, main page]

Skip to content

allow textfile to be overwritten on windows #650

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

Merged
merged 4 commits into from
May 11, 2021

Conversation

bluppfisk
Copy link
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
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
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
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
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
0