scttnlsn / dandelion

Incremental Git repository deployment.
http://scttnlsn.github.io/dandelion
MIT License
738 stars 60 forks source link

Add option to preserve symlinks #100

Closed dphoyes closed 7 years ago

dphoyes commented 9 years ago

Previous behaviour was to resolve each symlink locally, and reupload the referenced files. However, this fails if a symlink points to a directory. For me, the desired behaviour is to make no attempt to resolve symlinks, and preserve them in their original form by creating identical symlinks on the remote computer.

This behaviour can now be enabled by setting preserve_symlinks to true in the config file. By default, the behaviour should be unchanged though. Currently only the SFTP adapter is supported by this parameter, though it should be possible to make it work for others too.

Apologies for the large changeset; please let me know if anything should be implemented differently.

scttnlsn commented 9 years ago

I think what you've described is definitely a better way to handle symlinks. In addition to the directory problem you mentioned, the current solution is also suboptimal since it does not track changes to externally linked files. With the current solution I see no clear advantage over manually uploading symlinked files and I'm inclined to remove the existing functionality completely.

I think your solution could also be made to work with the FTP adapter but not for S3 (and perhaps some future adapters). What should happen in that case? Do you think the adapter should implement a no-op symlink method? Or should we try and upload the linked file?

Since there are various different cases to handle and the desired behavior is potentially different depending on the use case I'm wondering if symlink handling should be left out of Dandelion all together. Could you accomplish your desired symlink setup with a larger deploy process/script (part of which would be a dandelion deploy)?

dphoyes commented 9 years ago

For me, dandelion is a tool which allows me to perform the equivalent of a remote git checkout on servers where running git directly is not permitted. Since the behaviour of a true git checkout is to preserve links as this changeset does, I would consider dandelion incomplete if it didn't do the same. Any alternative, non git-like behaviours would possibly be better placed in a separate deployment tool, since it wouldn't need knowledge of the git repository and would have little overlap with what dandelion does.

I guess it's a bit trickier for adapters which don't allow symlinks. Since I don't have a personal use case for the S3 adapter, it's hard to say for sure, but I'd suggest that a project which uses S3 for deployment is unlikely to make use of symlinks, so a no-op (with a printed warning) is probably sufficient.

If it turns out that someone has a use for symlinks with incompatible adapters, another possibility is to simply upload a placeholder plain-text file for each symlink (containing the linked path, similar to dandelion's early behaviour), maybe with a '.dandelion-symlink' extension added. Then, any post-processing script would at least be able to see what the symlinks would have pointed to, and make any desired corrections.

dphoyes commented 7 years ago

Sorry to dig this up again, but I've made another attempt at this!

Please let me know if there's anything I can improve.

scttnlsn commented 7 years ago

Nice. This probably makes much more sense (the existing symlink handling is a little janky). I don't actually rely on the symlink behavior myself. Anyone else able to weigh in here?

scttnlsn commented 7 years ago

Do you think the old behavior should be used if the adapter does not respond to symlink?

dphoyes commented 7 years ago

I'm guessing that would only really be useful if it solved the issues with the old implementation. It would need to automatically re-upload any file targeted by a link if the targeted file has changed. To support links to directories, it would need to recursively upload the contents of the targeted directory, and hope there isn't a symlink loop! I'm not sure there's actually a use case for all this though...

scttnlsn commented 7 years ago

Good point. Existing behavior is kind of broken. I'll leave this open for a little bit and see if anyone else chimes in. Otherwise I think this looks good to merge. Mind squashing the commits?

Thank you!

dphoyes commented 7 years ago

Squashed and rebased. Thanks!

scttnlsn commented 7 years ago

OK, I'm thinking this will go into v0.5 as is and we can add additional symlink behavior down the road if folks are missing something. Sound good?

dphoyes commented 7 years ago

Sounds good to me. Arguably the proposal to remove the old behaviour was first made here 2 years ago, which is probably enough time!

scttnlsn commented 7 years ago

Available in v0.5