GitLab Web IDE commits ignore LFS

Got this warning when checking out the repository after 3ac9e793 was merged:

Encountered 1 file(s) that should have been pointers, but weren't:                                         
    content/blog/tor-censorship-in-russia/lead.png

It turns out that when creating a commit using the Web IDE, LFS is just ignored:

LFS files can be rendered and displayed but they cannot be updated and committed using the Web IDE. If an LFS file is modified and pushed to the repository, the LFS pointer in the repository is overwritten with the modified LFS file content.

Since we track image assets using LFS in the repository, this puts into question the use of the Web IDE as part of the authoring process, unfortunately...

I fixed this error using the command git lfs migrate import --include="*.png" and pushing.

added Icebox label

mentioned in merge request !20 (merged)

mentioned in merge request !52 (merged)

@tpo/tpa should we get this out of the icebox? It seems this is a needed issue to get resolve to have a smoothier workflow on posting to the blog?

I'm not sure what tpa could do about it aside from patching gitlab, or finding a different workaround. Patching the code would be great, but that would be a lot of work.

i also don't think we can fix this in gitlab directly without getting into patching it, which seems like a fool's errand at this point.

that doesn't mean we can't "fix" fix this though :) one idea i have is that we could have a CI job that would accept such MRs and fix them directly. some bot that would have credentials to push to the repo (basically with a deploy key) that could trigger when it finds a damaged LFS repo. it would work on the pull request and amend it to do the right thing. we could trigger it by doing a /foo command or something.

alternatively, it could just be in the gitlab-ci.yml file as well...

Anything that could help @lavamind on not having to do manual repetitive work related to fixing commits.

@anarcat Good points! I like the idea of a CI job that fixes LFS issues.

assigned to @lavamind

I'm not sure if giving CI write permissions on the repository is a great idea, security wise. It might be a bit less of an issue if we could grant those permissions on non-main branches but I don't see this option when creating deploy keys with write-access. Plus, one of the main points of LFS is to avoid binary files entering the git history in the first place, so if we simply add a follow-up commit that removes the file and add it to LFS, then the binary file still exists in the repository, forever.

That said, it's worthwhile to have a CI job that at least detects this in merge requests: if a fixup commit is made in the MR branch and the branch is squashed before merging, this should help the main branch to stay clean from non-LFS commits.

Yeah, that's what i had in mind: the bot would amend the merge request to clean stuff up, it wouldn't just add a new commit, obviously. :)

...

On 2022-06-02 14:41:04, Jérôme Charaoui (@lavamind) wrote:

Jérôme Charaoui commented:

I'm not sure if giving CI write permissions on the repository is a great idea, security wise. It might be a bit less of an issue if we could grant those permissions on non-main branches but I don't see this option when creating deploy keys with write-access. Plus, one of the main points of LFS is to avoid binary files entering the git history in the first place, so if we simply add a follow-up commit that removes the file and add it to LFS, then the binary file still exists in the repository, forever.

That said, it's worthwhile to have a CI job that at least detects this in merge requests: if a fixup commit is made in the MR branch and the branch is squashed before merging, this should help the main branch to stay clean from non-LFS commits.

-- Antoine Beaupré torproject.org system administration

added Next label and removed Icebox label

As an aside, GitLab is planning to replace the current WebIDE implementation with VSCode, and it's unclear if or how this might affect LFS commits.

added Doing label and removed Next label

mentioned in commit tpo/tpa/wiki-replica@a5644fd7

I did some more tests today and I think I finally figured out a good workaround: when files are added through the Web IDE to an existing branch in the project, then the relevant files are added via LFS as expected!

The problem appears when LFS files are added and a new branch is being created at the same time, via the web interface.

I've adjusted the blog instructions with an extra step at the beginning where users manually create a new branch via the web interface, then enter the Web IDE to add/edit files.

closed

mentioned in commit tpo/tpa/ci-templates@396655d2

is it work pushing some docs upstream on this?

I'm not really sure I want to embark on pushing updates to the GitLab documentation here. Maybe their limitations statement is a little bit too broad, but I'm not sure I can provide a much more accurate description of what's going on. For all we know it could be working by accident.

fair enough

...

On 2022-06-08 15:06:03, Jérôme Charaoui (@lavamind) wrote:

Jérôme Charaoui commented on a discussion: #40036 (comment 2811049)

I'm not really sure I want to embark on pushing updates to the GitLab documentation here. Maybe their limitations is a little bit too broad, but I'm not sure I can provide a much more accurate description of what's going on. For all we know it could be working by accident.

-- Antoine Beaupré torproject.org system administration