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.

Sign up for GitHub

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

Jump to bottom

doc: add note for fs.watch on virtualized env #6809

Closed
wants to merge 1 commit into from
Closed

doc: add note for fs.watch on virtualized env #6809

wants to merge 1 commit into from

Conversation

eljefedelrodeodeljefe
Copy link
Contributor

Checklist
  • tests and code linting passes
  • a test and/or benchmark is included
  • documentation is changed or added
  • the commit message follows commit guidelines
Affected core subsystem(s)

doc

Description of change

On virtualization systems fs.watch doesn't work reliably, which is implicitly already in the respective docs. To make it more explicit this PR adds a subsentence. The following statement defers to use of fs.watchFile, which is the potential workaround.

Fixes: #6765

@eljefedelrodeodeljefe eljefedelrodeodeljefe added doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. labels May 17, 2016
@eljefedelrodeodeljefe eljefedelrodeodeljefe mentioned this pull request May 17, 2016
Closed
@cjihrig
Copy link
Contributor

cjihrig commented May 17, 2016

LGTM

1 similar comment
@thefourtheye
Copy link
Contributor

LGTM

jasnell
jasnell reviewed May 17, 2016
View reviewed changes
doc/api/fs.md
reliably or at all.
directories on network file systems (NFS, SMB, etc.) and on exposed host file
systems when using virtualization software (Vagrant, Docker, etc.) often doesn't
work reliably or at all.

Copy link
Member

@jasnell jasnell May 17, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

minor nit... I'd restructure the sentence just a bit...

Something like:

For example, watching files or directories can be unreliable, and in some
cases impossible, on network file systems (NFS, SMB, etc), and on host
file systems when using virtualization software such as Vagrant, Docker, etc.

But I'm good with this also ;-)

@jasnell
Copy link
Member

jasnell commented May 17, 2016

Left one comment but generally LGTM

@eljefedelrodeodeljefe
Copy link
Contributor Author

Fine for me. Will adapt it before landing. Thx

@mhdawson
Copy link
Member

I'm + 1for @jasnell's version, but generally LGTM as well.

bnoordhuis
bnoordhuis reviewed May 18, 2016
View reviewed changes
doc/api/fs.md
@@ -1180,8 +1180,9 @@ to be notified of filesystem changes.

If the underlying functionality is not available for some reason, then
`fs.watch` will not be able to function. For example, watching files or
directories on network file systems (NFS, SMB, etc.) often doesn't work
reliably or at all.
directories on network file systems (NFS, SMB, etc.) and on exposed host file
Copy link
Member

Choose a reason for hiding this comment

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

s/and on/or/? I suspect most programmers are like me and interpret it boolean logic-style.

@bnoordhuis
Copy link
Member

LGTM with a suggestion.

@eljefedelrodeodeljefe
doc: add note for fs.watch virtualized env
3d5fe13 
Fixes: #6765
PR-URL: #6809
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
eljefedelrodeodeljefe added a commit that referenced this pull request May 19, 2016
@eljefedelrodeodeljefe
doc: add note for fs.watch virtualized env
ae0f68d 
Fixes: #6765
PR-URL: #6809
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@eljefedelrodeodeljefe
Copy link
Contributor Author

Went ahead and landed it in ae0f68d

Addressed both @bnoordhuis an @jasnell nits

For example, watching files or
directories can be unreliable, and in some cases impossible, on network file
systems (NFS, SMB, etc), or host file systems when using virtualization software
such as Vagrant, Docker, etc.

Fishrock123 pushed a commit that referenced this pull request May 23, 2016
@eljefedelrodeodeljefe @Fishrock123
doc: add note for fs.watch virtualized env
6d71cbc 
Fixes: #6765
PR-URL: #6809
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
rvagg pushed a commit that referenced this pull request Jun 2, 2016
@eljefedelrodeodeljefe @rvagg
doc: add note for fs.watch virtualized env
b52d838 
Fixes: #6765
PR-URL: #6809
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@MylesBorins MylesBorins mentioned this pull request Jun 2, 2016
Closed
MylesBorins pushed a commit that referenced this pull request Jun 3, 2016
@eljefedelrodeodeljefe
doc: add note for fs.watch virtualized env
ccae698 
Fixes: #6765
PR-URL: #6809
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@MylesBorins MylesBorins mentioned this pull request Jun 24, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Jun 24, 2016
@eljefedelrodeodeljefe
doc: add note for fs.watch virtualized env
2b0f40c 
Fixes: #6765
PR-URL: #6809
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request Jun 24, 2016
@eljefedelrodeodeljefe
doc: add note for fs.watch virtualized env
062009c 
Fixes: #6765
PR-URL: #6809
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@eljefedelrodeodeljefe @cjihrig @thefourtheye @jasnell @mhdawson @bnoordhuis @MylesBorins