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

Remove file_windows.go #7845

Merged
merged 1 commit into from
Jul 4, 2023
Merged

Conversation

manuelbuil
Copy link
Contributor

Proposed Changes

Removes the file_windows.go file and changes file.go so that it is included in windows builds

Types of Changes

Bug fix

Verification

Run k3s on windows/linux and make sure things work (e.g. the password file which is written by using these functions)

Testing

Linked Issues

#7844

User-Facing Change


Further Comments

Signed-off-by: Manuel Buil <mbuil@suse.com>
@manuelbuil manuelbuil requested a review from a team as a code owner July 3, 2023 14:12
@codecov
Copy link

codecov bot commented Jul 3, 2023

Codecov Report

Patch coverage has no change and project coverage change: +31.68 🎉

Comparison is base (0809187) 19.85% compared to head (d593c83) 51.53%.

Additional details and impacted files
@@             Coverage Diff             @@
##           master    #7845       +/-   ##
===========================================
+ Coverage   19.85%   51.53%   +31.68%     
===========================================
  Files          83      143       +60     
  Lines        7686    14512     +6826     
===========================================
+ Hits         1526     7479     +5953     
+ Misses       5930     5846       -84     
- Partials      230     1187      +957     
Flag Coverage Δ
e2etests 49.45% <ø> (?)
inttests 44.49% <ø> (?)
unittests 19.85% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/util/file.go 21.05% <ø> (+10.52%) ⬆️

... and 117 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@dereknola
Copy link
Member

dereknola commented Jul 3, 2023

I would test RKE2 and make sure this doesn't affect it.

Edit: I see the original creation is only around k3s, not rke2/windows support.

@manuelbuil
Copy link
Contributor Author

manuelbuil commented Jul 4, 2023

I would test RKE2 and make sure this doesn't affect it.

Edit: I see the original creation is only around k3s, not rke2/windows support.

Yes. I tested this with simple go files and it works correctly. Of course the file modes are different in windows and linux, like 0644 has no meaning in Windows. So for the functions that do chmod, the functions calling should differentiate between windows and linux, or we should replace those functions with something like:

  • SetFileModeForPath ===> SetReadOnlyFileModeForPath

And then, depending on the OS use 0400 or 0200 (off the top of my head)

Currently, for windows we were doing nothing and returning err = nil, which is what happens if you pass a mode which does not make sense in Windows (e.g. 0644), so we are not changing any behaviour. Besides, k3s is not supporting windows, so the impact is minimal

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.

5 participants