Skip to content
This repository has been archived by the owner on Nov 22, 2022. It is now read-only.

fix(config): use ioutil.WriteFile instead of renameio #211

Closed
wants to merge 1 commit into from

Conversation

andhe
Copy link
Contributor

@andhe andhe commented Sep 7, 2020

As reported apparently renameio does not work on windows and there's
no obvious way to even make an atomic file overwrite on windows at all,
thus renameio is looking at dropping Windows support.

This change simply switches to ioutil.WriteFile which is not atomic
but the code should still be nicer than the previous one we replaced.

While at it also cut down on the readability part of the files
permissions as the config file could contain sensitive information
(like the token).

Fixes #210

As reported apparently renameio does not work on windows and there's
no obvious way to even make an atomic file overwrite on windows at all,
thus renameio is looking at dropping Windows support.

This change simply switches to ioutil.WriteFile which is not atomic
but the code should still be nicer than the previous one we replaced.

While at it also cut down on the readability part of the files
permissions as the config file could contain sensitive information
(like the token).

Fixes profclems#210
@pull-assistant
Copy link

pull-assistant bot commented Sep 7, 2020

Score: 1.00

Best reviewed: commit by commit


Optimal code review plan

     fix(config): use ioutil.WriteFile instead of renameio

Powered by Pull Assistant. Last update 427ba7b ... 427ba7b. Read the comment docs.

Copy link
Owner

@profclems profclems left a comment

Choose a reason for hiding this comment

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

I don't know if this is a right decision to make the atm.

I'm thinking the need of importing/linking runtime (and renameio on windows) could be avoided by creating a WriteFile helper method in a file which has a !+build windows and another writefile_windows.go which implements WrifeFile using ioutil.WriteFile.

This looks like a fairly optimal solution for me.

@andhe
Copy link
Contributor Author

andhe commented Sep 7, 2020

Ok, published #212 as a replacement for this.

@andhe andhe closed this Sep 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failed to update config file on windows
2 participants