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

fix fsnotify watcher not fully working #2258

Merged
merged 1 commit into from
Oct 29, 2023

Conversation

wjiec
Copy link
Contributor

@wjiec wjiec commented Oct 23, 2023

When modifying the views.yml file with vim, the event reported by fsnotify is RENAME, which results in the loss of the watcher on the original file. References fsnotify.Watcher.Add comment:

// # Watching files
//
// Watching individual files (rather than directories) is generally not
// recommended as many tools update files atomically. Instead of "just" writing
// to the file a temporary file will be written to first, and if successful the
// temporary file is moved to to destination removing the original, or some
// variant thereof. The watcher on the original file is now lost, as it no
// longer exists.

@wjiec
Copy link
Contributor Author

wjiec commented Oct 23, 2023

I was puzzled when testing #2253 because I changed the config file but the code didn't take effect. So I tried to find the reason. 🤔️

Copy link
Owner

@derailed derailed left a comment

Choose a reason for hiding this comment

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

@wjiec Nice catch Jayson! Thank you for this update!!

@derailed derailed merged commit 09719eb into derailed:master Oct 29, 2023
2 of 3 checks passed
@wjiec wjiec deleted the bugfix/reload-config branch October 30, 2023 01:34
@derailed derailed mentioned this pull request Nov 8, 2023
placintaalexandru pushed a commit to placintaalexandru/k9s that referenced this pull request Apr 3, 2024
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