Add support for Git LFS enabled repositories (fixes #1853) #1941

jannfis · 2019-07-15T19:43:05Z

This PR enables LFS (Large File Support) for repositories and fixes #1853

For this, a new switch --enable-lfs is introduced to repo add command. LFS must be enabled on a per repository basis, and it should be used carefully (because LFS can reference really large files).

For declarative setup, the property name is enableLfs and of type bool.

As a bonus, while I was at the code, I fixed upsert mechanism for the insecure-ignore-host-key and insecure-skip-server-validation setting of repositories as well.

…3-add-git-lfs-support

jannfis · 2019-07-15T19:48:26Z

util/git/client.go

@@ -361,9 +395,8 @@ func (m *nativeGitClient) runCredentialedCmd(command string, args ...string) (st
 func (m *nativeGitClient) runCmdOutput(cmd *exec.Cmd) (string, error) {
 	cmd.Dir = m.root
 	cmd.Env = append(cmd.Env, os.Environ()...)
-	cmd.Env = append(cmd.Env, "HOME=/dev/null")
-	cmd.Env = append(cmd.Env, "GIT_CONFIG_NOSYSTEM=true")
-	cmd.Env = append(cmd.Env, "GIT_CONFIG_NOGLOBAL=true")


I have removed these environment variables, since I was assuming these were set to not fiddle with Git configuration when the argocd CLI still tested repository access from the client it was executed. This behaviour was removed with PR #1807, and the git is not called on the client anymore.

I think this is right assumption and we don't need these variables anymore. @jessesuen can you confirm please?

alexmt

LGTM. @jessesuen can you take a look too? Looking for confirmation about GIT_CONFIG_NOSYSTEM, GIT_CONFIG_NOGLOBAL env variables

jannfis · 2019-07-17T20:10:02Z

I just added tests for the PR that references a LFS enabled mock repository in my GitHub account from https://github.com/jannfis/argocd-testrepo-lfs - if you want, you can clone that repository to argocd GitHub account and I will change the reference in the code.

alexmt · 2019-07-17T20:23:55Z

Forked test repo to https://github.com/argoproj-labs/argocd-testrepo-lfs

jannfis · 2019-07-17T20:32:23Z

Hm, I can't seem to get the LFS plugin for Git into the image run during CircleCI workflow. I updated base image in Dockerfile, but that does not seem to work. Does it work differently for CircleCI? Do I have to perform additional steps?

alexmt · 2019-07-17T20:45:52Z

Ah, right. I need to update builder image and push it. Doing it right now. Can you please change image to argoproj/argo-cd-ci-builder:v1.0.1 here https://github.com/argoproj/argo-cd/blob/master/test/fixture/testrepos/start-git.sh#L5-L5 ?

alexmt · 2019-07-17T21:30:02Z

util/git/client.go

@@ -41,14 +41,15 @@ type Client interface {
 	Checkout(revision string) error
 	LsRemote(revision string) (string, error)
 	LsFiles(path string) ([]string, error)
+	LsLargeFiles() ([]string, error)


There is one more struct which supposed to implement git client interface: https://github.com/argoproj/argo-cd/blob/master/reposerver/metrics/gitwrapper.go#L1-L1

Yes, one little change, so much butterfly effect. Should have compiled all components before pushing that commit :|

jannfis · 2019-07-17T21:30:09Z

I missed some additional changes in Git Client interface which I introduced for the tests with commit
e94726b, which broke the e2e tests (and the reposerver, d'oh). Will fix that tomorrow. Meanwhile, can't figure out why the build is still failing.

alexmt · 2019-07-17T22:39:34Z

util/git/git_test.go

@@ -139,6 +140,37 @@ func TestLsRemote(t *testing.T) {
 	}
 }

+// Running this test requires git-lfs to be installed on your machine.
+func TestLFSClient(t *testing.T) {
+	tempDir, err := ioutil.TempDir("", "git-client-lfs-test-")


Looks like we will need to install git-lfs in circle ci config:

https://discuss.circleci.com/t/is-git-lfs-supported-by-circleci-2-0/11283/7

name: Install Git LFS command: | curl -s https://packagecloud.io/install/repositories/github/git-lfs/script.deb.sh | bash apt-get update apt-get install -y git-lfs openssh-client

Thanks a lot @alexmt - I got CircleCI to install & use the Git LFS plugin now. Unfortunately, this led to another error, which I believe is CircleCI specific. The LFS test fails with:

Error Trace: git_test.go:162 Error: Received unexpected error: `git lfs fetch --all` failed exit status 2: batch request: Invalid deploy key for argoproj-labs/argocd-testrepo-lfs: exit status 1 error: failed to fetch some objects from 'https://github.com/argoproj-labs/argocd-testrepo-lfs.git/info/lfs' Test: TestLFSClient

I changed test repositories, but to no avail A quick search on Google revealed not much to this specific error, and all I found seems to be CircleCI related. The LFS works from multiple machines I tested on, just not in the CircleCI build. I will look through their support base for possible solutions, or write to their support.

We are going to dockerize tests since it's been pretty painful to configure Circle CI same as real production environment. I would suggest moving your test to test/e2e directory and skip it during CI run using test.LocalOnly(t) (example)

After dockerizing the tests we will enable this test in CI

I have found the reason and could fix it. CircleCI injects a SSH key to the user's $HOME/.ssh, which is reason for LFS failing (doesn't like deploy keys). Actually, I guess that was one of the reasons why HOME was set to /dev/null in Git client implementation -- and I removed that with one of the previous commits. It's back in place now, LFS now also works in the CI environment.

jannfis · 2019-07-18T16:40:52Z

Ah, right. I need to update builder image and push it. Doing it right now. Can you please change image to argoproj/argo-cd-ci-builder:v1.0.1 here https://github.com/argoproj/argo-cd/blob/master/test/fixture/testrepos/start-git.sh#L5-L5 ?

I had to switch back to v1.0.0 because the v1.0.1 seems broken - it apperently has no goreman included. Output from make start-e2e:

18:39:53  git-server | bash: goreman: command not found
18:39:53  git-server | Terminating git-server

…3-add-git-lfs-support

jannfis · 2019-07-18T18:42:17Z

Following E2E test is failing: TestCRDs - e2e - ran fine two cycles ago, also runs fine locally. I suspect hiccup/timeout.

From my point of view, this PR is ready for merge at your discretion.

Thanks a lot for your help and shared experience, @alexmt !

alexmt · 2019-07-18T19:49:39Z

Awesome! Thank you, @jannfis. You are right about e2e test failure. We've just found it is due to a real bug: #1940

LGTM

jannfis added 2 commits July 15, 2019 21:36

Add support for LFS enabled repositories

b757175

Merge branch 'master' of https://github.com/argoproj/argo-cd into 185…

79b0ff4

…3-add-git-lfs-support

jannfis commented Jul 15, 2019

View reviewed changes

jannfis mentioned this pull request Jul 17, 2019

Support Client HTTPS certifcates for private git repositories #1945

Closed

alexmt requested a review from jessesuen July 17, 2019 18:01

alexmt approved these changes Jul 17, 2019

View reviewed changes

Adds tests and fixes a small bug.

e94726b

The CI image also needs git-lfs

2d07745

jannfis added 2 commits July 17, 2019 22:50

Change URL of test repo & version of CI builder image

13e8716

Change name of local temp path to be more consistent

5179079

alexmt reviewed Jul 17, 2019

View reviewed changes

jannfis added 10 commits July 18, 2019 16:25

Format the test a little more nicely

72720f3

Implement missing LsLargeFiles() method in gitClientWrapper

f7a8003

Rollback to cicd image v1.0.0 (1.0.1 seems broken)

6d22909

Install Git LFS plugin for CircleCI workflows

a5e8f63

Git LFS installations needs privileges

aba3318

Not the curl needs privileges, but the shell executing the script

9a2cf4e

Add new mock method for LsLargeFiles() via mockery

39fbf8b

Also pull in openssh-client for Git LFS

f916d12

Dirty workaround for sticky apt-get processes in CircleCI image

769a44f

Testing another repo for LFS tests

011b870

jannfis added 2 commits July 18, 2019 19:45

Set $HOME back to /dev/null when executing Git

e16048d

Change back LFS test repo URL and pre-commit changes

f3d4495

Merge branch 'master' of https://github.com/argoproj/argo-cd into 185…

30ee826

…3-add-git-lfs-support

alexmt merged commit 8f3a604 into argoproj:master Jul 18, 2019

alexec added this to the v1.2 milestone Jul 24, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Git LFS enabled repositories (fixes #1853) #1941

Add support for Git LFS enabled repositories (fixes #1853) #1941

jannfis commented Jul 15, 2019

jannfis Jul 15, 2019

alexmt Jul 17, 2019

alexmt left a comment

jannfis commented Jul 17, 2019

alexmt commented Jul 17, 2019

jannfis commented Jul 17, 2019

alexmt commented Jul 17, 2019

alexmt Jul 17, 2019

jannfis Jul 17, 2019

jannfis commented Jul 17, 2019

alexmt Jul 17, 2019

jannfis Jul 18, 2019

alexmt Jul 18, 2019

jannfis Jul 18, 2019

jannfis commented Jul 18, 2019

jannfis commented Jul 18, 2019

alexmt commented Jul 18, 2019

Add support for Git LFS enabled repositories (fixes #1853) #1941

Add support for Git LFS enabled repositories (fixes #1853) #1941

Conversation

jannfis commented Jul 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexmt left a comment

Choose a reason for hiding this comment

jannfis commented Jul 17, 2019

alexmt commented Jul 17, 2019

jannfis commented Jul 17, 2019

alexmt commented Jul 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannfis commented Jul 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannfis commented Jul 18, 2019

jannfis commented Jul 18, 2019

alexmt commented Jul 18, 2019