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

fix(lfs): fixes lfs pointers in editorial workflow #4613

Closed

Conversation

LukasGentele
Copy link

Summary
Without this commit, the CMS would alter the pointer files for each large media file whenever you edit a draft in editorial mode. The problem is that the filesize, blob and name are not available at the time when a commit is being generated which leads to all pointer files looking like this when you edit a draft that already contained an LFS object (i.e. first time adding an image works but editing text afterward will lead to this issue):

version https://git-lfs.github.com/spec/v1
oid sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855      # = hash of ""  because blob is empty
size 0

This PR fixes this problem by skipping all media files that do not have a name. Alternatively, there could be check for size == 0 instead.

I also thought about loading the correct metadata and blobs in the unpublishedEntryMediaFile in backend-git-gateway but that causes additional loading time every time someone opens a draft, so I believe skipping the commits when saving a draft is the better option. Happy to make adjustments to this though if needed. I'm really looking forward for this to be fixed because this bug makes using editorial workflow with images in LFS a huge pain.

Steps to reproduce the issue:

  1. Create a netlify site with netlify lfs (and in my case git-gateway provided by netlify and hooked up for github but it should affect other backends as well)
  2. Enable editorial_workflow
  3. Create a draft and add an image in the content which matches the LFS pattern
  4. Save the draft
  5. Edit the draft
  6. Save the draft again => now the CMS will change 2 files in the commit. Instead of just changing the markdown file, it will also set the LFS pointer for the image to the zero-pointer as shown in the code snippet above.

Test plan
Ran yarn test and tested the behavior with a netlify-hosted blog page using git-gateway backed by github and netlify-lfs.

Thanks for looking into this. Netlify CMS is awesome!

A picture of a cute animal (not mandatory but encouraged)

@LukasGentele LukasGentele requested a review from a team November 20, 2020 06:06
@erezrokah erezrokah added the type: bug code to address defects in shipped code label Nov 24, 2020
@erezrokah erezrokah self-requested a review November 26, 2020 15:32
@erezrokah
Copy link
Contributor

erezrokah commented Dec 6, 2020

Hi @LukasGentele, thank you for this and sorry for the late reply.
I've created another PR to fix this #4678
We do need the metadata as explained in that PR, the reason the fix in this PR is working is due to another bug.

To reproduce the other bug:

  1. Save a large media file in editorial workflow
  2. Open the media library and remove that file
  3. Save the entry again
  4. See that the file is still present on the PR branch

The linked PR fixes both bugs.

@erezrokah erezrokah closed this Dec 6, 2020
@LukasGentele
Copy link
Author

@erezrokah Makes sense. Thanks for the fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug code to address defects in shipped code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants