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.

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

Calculation of local_files_to_upload is incorrect #19

Closed
kranzky opened this issue Nov 9, 2011 · 9 comments
Closed

Calculation of local_files_to_upload is incorrect #19

kranzky opened this issue Nov 9, 2011 · 9 comments

Comments

@kranzky
Copy link

kranzky commented Nov 9, 2011

In AssetSync::Storage::upload_files, the list of files to upload is calculates as follows:

local_files_to_upload = (remote_files | local_files) - (remote_files & local_files)

This means that asset_sync will attempt to upload files that are present in remote_files but which are not present in local_files. This will break if, for example, the assets directory is cleaned, but old versions of assets are being kept on S3. Perhaps this should be changed to:

local_files_to_upload = local_files - remote_files
@kranzky
Copy link
Author

kranzky commented Nov 9, 2011

Similarly, shouldn't this:

from_remote_files_to_delete = (local_files | remote_files) - (local_files & remote_files)

be changed to:

from_remote_files_to_delete = remote_files - local_files

Otherwise, we'll attempt to delete files that don't yet exist on S3?

@kranzky
Copy link
Author

kranzky commented Nov 9, 2011

If these changes are made, then Storage::sync should upload new files before deleting old files, otherwise it will delete files that are about to be uploaded before uploading them.

davidjrice added a commit that referenced this issue Nov 14, 2011
@davidjrice
Copy link
Contributor

Thanks! Version 0.1.10 should solve this. Let me know if there are any further issues.

@levent
Copy link
Contributor

levent commented Mar 20, 2012

This seems to only decide which files to upload based on filename and not whether the contents have changed?
Is there a way to either:

  1. upload local files to remote, if they actually differ.
  2. upload all local files, regardless of whether they exist remotely.

@kranzky
Copy link
Author

kranzky commented Mar 20, 2012

@levent we're using this with sprockets + hashed filenames, so the filename differs whenever its content changes. it's ideal to do this for static files, as it allows files to be cached indefinitely.

@levent
Copy link
Contributor

levent commented Mar 20, 2012

@jasonhutchens we do the same.

However, we also have some static files 500.html for example that just refer to application.css as we cannot update them every commit to point to the correct fingerprinted asset.

Unfortunately that means that application.css is always out of date, unless we delete it from S3 prior to running asset_sync.

I imagine others do the same within static files or on remote apps that include js / css from remote apps using the asset pipeline.

@davidjrice
Copy link
Contributor

Guys.

Digest assets are the way forward here.

To check the digests of the assets (on S3) themselves when uploading
is very inefficient. It would mean downloading the asset to digest the
content.

This is redundant if the asset has a digest in it's file name.

I understand it is useful to reference assets staticly, however I
think this is bad practice

I have started on a pull request into Rails to completely move the
rest of public dir into the asset pipeline. There are some pieces we
need to improve before this can be merged in. However this will allow
us to precompile 404.html, 500.html etc

Sound good?

Other things that we could do would be an asset_sync "clean" rake
task. Or a task to delete any nondigest assets.

Sent from my tiny Internet machine

On 20 Mar 2012, at 14:23, Levent Ali
reply@reply.github.com
wrote:

@jasonhutchens we do the same.

However, we also have some static files 500.html for example that just refer to application.css as we cannot update them every commit to point to the correct fingerprinted asset.

Unfortunately that means that application.css is always out of date, unless we delete it from S3 prior to running asset_sync.

I imagine others do the same within static files or on remote apps that include js / css from remote apps using the asset pipeline.


Reply to this email directly or view it on GitHub:
#19 (comment)

@levent
Copy link
Contributor

levent commented Mar 20, 2012

David,

I agree 100% on moving the rest of the public directory into the asset pipeline.

However, in the meantime some sort of "clean before compile" rake task would be very useful for us in production.
I believe there could always be some cases where remote apps still need to reference a non-changing asset filename, even when your changes make it into Rails.

I could look at submitting a pull request instead of just grumbling about it here though :)

Cheers
L

@davidjrice
Copy link
Contributor

Cool cool. I'm in the mountains at the moment and will look at this next week!

Sent from my tiny Internet machine

On 20 Mar 2012, at 19:05, Levent Ali
reply@reply.github.com
wrote:

David,

I agree 100% on moving the rest of the public directory into the asset pipeline.

However, in the meantime some sort of "clean before compile" rake task would be very useful for us in production.
I believe there could always be some cases where remote apps still need to reference a non-changing asset filename, even when your changes make it into Rails.

I could look at submitting a pull request instead of just grumbling about it here though :)

Cheers
L


Reply to this email directly or view it on GitHub:
#19 (comment)

jasukkas pushed a commit to AdsOptimal/asset_sync that referenced this issue Sep 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants