-
Notifications
You must be signed in to change notification settings - Fork 6
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
make lsyncd delete-option configurable #7
base: master
Are you sure you want to change the base?
Conversation
<%- else -%> | ||
delete = "<%= @delete %>", | ||
<%- end -%> | ||
<%- end -%> |
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.
Thanks for the PR! I really appreciate you taking the time.
The only part I can't quite follow is this block (and the similar block in rsyncssh.conf.lua.erb
). Is there a reason we can't just add delete = <%= @delete %>
to the templates without the surrounding conditionals?
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.
Hello Nic - I appreciate your willingness to look into it.
The reason for the first conditional was to cause no change in config files for existing configurations. As true
is the default I opted to not print it then.
As for the second conditional, reading the doc it seems to me the first two options are boolean, the former ones string.
I just tested to include the modes running/startup
unquoted and they do not follow their advertised behaviour - i.e. startup
is not supposed to delete on target during runtime, but will do so if the setting is placed unquoted (as it resolves to bool true
?). Same goes for printing true
quoted, it will not delete on target. Maybe I should look into lsyncd if they can all be made strings.
If the control structure / indentation looks awkward or introduces confusion and you prefer any other way, please change. To me the distinction boolean/string-option proved to be important and therefore at least one conditional separating the option types is needed.
This change makes the delete option of lsyncd configurable. As mentioned in the doc "many users requested exceptions for this, for various reasons".
My specific use-case is in compiled asset-files that can exist on an appserver, but not on the orchestrating masternode. Syncing the directory from the master as source gives me flexibility nonetheless - working around webapp limitations/browser caching effects and having current and old assetfiles.
You can see results of testing the false and startup option here. I'll need the false option mostly.
I had some difficulties to decide on proper indenting of the delete parameter in the .erb templates, if you have any suggestions I'd be thankful. For now I decided to cause no changes for existing configurations.