-
-
Notifications
You must be signed in to change notification settings - Fork 424
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
YAML files: ignore files inside automation sub-folder #4291
Conversation
Also ignore unreachable files Signed-off-by: Laurent Garnier <lg.hc@free.fr>
Handles user feedbacks testing OH 4.2 milestone 4. First user reported these logs:
Second user reported this exception:
|
@@ -99,7 +99,8 @@ public YamlModelRepositoryImpl(@Reference(target = WatchService.CONFIG_WATCHER_F | |||
|
|||
// read initial contents | |||
try (Stream<Path> files = Files.walk(watchPath)) { | |||
files.filter(Files::isRegularFile).map(watchPath::relativize).forEach(f -> processWatchEvent(CREATE, f)); | |||
files.filter(f -> Files.isReadable(f) && Files.isRegularFile(f)).map(watchPath::relativize) |
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.
Is it really necessary to check .isReadable
here? The check is done again when processWatchEvent
is called.
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.
I was not sure what call triggered the AccessDeniedException. Not really clear when looking at the stack trace.
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.
lgtm, thanks!
@@ -116,7 +117,8 @@ public void deactivate() { | |||
public synchronized void processWatchEvent(Kind kind, Path path) { | |||
Path fullPath = watchPath.resolve(path); | |||
String pathString = path.toString(); | |||
if (Files.isDirectory(fullPath) || fullPath.toFile().isHidden() || !pathString.endsWith(".yaml")) { | |||
if (!Files.isReadable(fullPath) || Files.isDirectory(fullPath) || path.startsWith("automation") |
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.
@lolodomo, @J-N-K - even though it's not a root cause, do you think it would make sense for performance reasons to explicitly ignore ".git" directories as well? See https://community.openhab.org/t/openhab-4-2-milestone-discussion/154316/185
I realize this could quickly turn into a rather large blacklist, though.
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.
No. That doesn't make sense. Another guy in thread thread had a directory _infdb
, why should we add .git
but not _infdb
? automation
is different because it is readable, but may contain incompatible .yaml
files.
Also ignore unreachable files
Signed-off-by: Laurent Garnier lg.hc@free.fr