-
Notifications
You must be signed in to change notification settings - Fork 273
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
Remote sources fix watch #1071
Remote sources fix watch #1071
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,6 @@ | ||
project: | ||
name: test-project-ext-project-sources | ||
environmentDefaults: | ||
variables: | ||
some: variable | ||
environments: | ||
- name: local | ||
providers: | ||
- name: test-plugin | ||
- name: test-plugin-b | ||
- name: other |
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
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.
This basically means that the watcher will not work right if you add a new source while watching. That was okay(ish) when it was just the project root, but may be problematic now. I think it's fairly simple to solve though. We just need to keep track of which paths are currently being watched (or read that from the existing watcher if it exists) and add/remove as necessary.
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.
Yeah, you can just get a list of currently watched paths (if any), and reconcile that against the paths we want to watch: https://github.com/paulmillr/chokidar#methods--events
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.
Do you mean when you run the link / unlink commands while Garden is already running in watch mode?
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, more thinking of when you add a source in your project config while watching for changes.
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.
Ah, didn't think of that. However, you'd still need to link it (even if it's local), which is a separate command. So the assumption is still that the user would kill the running process, run
garden link
and then start it again.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.
Linking basically updates the
.garden/local-config.yml
file and I'm not sure if a running process will handle changes to that.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.
We could also treat that as a config change and watch that file, I suppose
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.
Yeah I guess. Wondering if there are any unwanted side effects since Garden updates this file programmatically. And not just for setting/removing linked sources. But I guess in any case you'd want to trigger a config change event.
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.
We could also special-case the particular variables in the config. But I'd suggest creating a separate issue for that.