Skip to content
This repository has been archived by the owner on Aug 3, 2023. It is now read-only.

Fix asset manifest logic to ensure uploaded files remain on Workers KV remote #635

Merged
merged 2 commits into from
Sep 20, 2019

Conversation

gabbifish
Copy link
Contributor

This PR fixes the bug observed yesterday, where wrangler publish --site seemed to publish nothing to the Workers KV remote.

The bug was a missing patch of logic related to wrangler publish --site's calling of sync().

When sync is called, it does two things:

  1. first, upload keys (using the asset manifest logic, file paths with their value hash appended)
  2. second, delete keys that exist on remote but not on local.

The bug was that during the second step, the local keys did not have the file's value hash appended, and wrangler perceived the keys on the local machine as JUST their pathnames. this means that during the second delete step of sync, all the files on the remote (which had just been uploaded) were deleted!
I have now successfully deployed a site from scratch using the wrangler init and wrangler publish --site logic. I made sure to nuke my old static assets namespace and start TOTALLY from scratch to ensure it works as expected.

@gabbifish gabbifish added the regression Something is broken, but works in previous releases label Sep 20, 2019
@ashleygwilliams ashleygwilliams modified the milestones: 1.4.0, KV Additions Sep 20, 2019
@EverlastingBugstopper
Copy link
Contributor

EverlastingBugstopper commented Sep 20, 2019

Ah - yes. This is a result of me not understanding what exactly the function was used for - this makes a lot of sense to me now.

edit: i guess it's more that i just didn't think about the fact that i was changing the behavior and the behavior also needed changing here

Copy link
Contributor

@EverlastingBugstopper EverlastingBugstopper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@ashleygwilliams ashleygwilliams merged commit e8bb966 into feat-kv-additions Sep 20, 2019
@ashleygwilliams ashleygwilliams deleted the gabbi/fix-asset-manifest-logic branch September 20, 2019 15:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
regression Something is broken, but works in previous releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants