-
Notifications
You must be signed in to change notification settings - Fork 110
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
Don't rename unless file is in same dir #603
Don't rename unless file is in same dir #603
Conversation
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #603 +/- ##
==========================================
+ Coverage 70.18% 70.54% +0.36%
==========================================
Files 10 10
Lines 2123 2122 -1
==========================================
+ Hits 1490 1497 +7
+ Misses 517 504 -13
- Partials 116 121 +5 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hard to test this one on GHA. Works for me at least.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing this! 🚀
@kipz - Thank you for testing this! 💯 My assumption is the examples that we run now both on windows and linux should be enough to catch potential errors of this, i.e. a temp file failed to be renamed. On that note, I'll go ahead and enable the macos environment too 👍 |
Here's the PR enabling the runner environments - #604. My assumption was we enabled the windows one for all, but it was just for the unit tests. Now it's going to run the examples too 👍 @jonnystoten - Now that I enabled the runners in the PR I see some of the examples for windows do fail. I don't have a windows machine in hand, do you think you can take a look while you're at this topic? I can give it a go using the CI later today or tomorrow too. |
Let's wait until the CI for windows is green.
This was originally done for windows only, but renaming can cause issues regardless of OS if the target dir is on another device. Signed-off-by: Jonny Stoten <jonny@jonnystoten.com>
6d2722f
to
aea0415
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we enabled the windows runner it manifested some errors which is not surprising since we haven't tested go-tuf-metadata at all on Windows. I've opened an issue about it if someone is interested in addressing this - #605.
I'm okay to accept this PR since it doesn't seem related to the issue I mention 👍
Hey thanks @rdimitrov ! I actually don't have a windows machine either but I could have a look using the CI to test, the failure I can see looks pretty straightforward. |
This was originally done for windows only, but renaming can cause issues regardless of OS if the target dir is on another device.