From f78090d899841259da39370f19d3817133a7697e Mon Sep 17 00:00:00 2001 From: jakobmoellerdev Date: Tue, 17 Dec 2024 12:19:48 +0100 Subject: [PATCH] chore: cleanup resolver/downloader because no update is needed --- .../extensions/accessmethods/git/method.go | 2 + api/tech/git/fs.go | 45 ++----- api/tech/git/logging.go | 2 - api/tech/git/resolver.go | 126 ++---------------- api/tech/git/resolver_test.go | 112 ++++++++++++++++ api/tech/git/suite_test.go | 13 ++ api/tech/git/testdata/repo/file_in_repo | 1 + .../accessio/downloader/git/downloader.go | 112 ---------------- api/utils/blobaccess/git/access.go | 2 +- api/utils/blobaccess/git/options.go | 3 - go.mod | 1 - go.sum | 2 - 12 files changed, 147 insertions(+), 274 deletions(-) create mode 100644 api/tech/git/resolver_test.go create mode 100644 api/tech/git/suite_test.go create mode 100644 api/tech/git/testdata/repo/file_in_repo delete mode 100644 api/utils/accessio/downloader/git/downloader.go diff --git a/api/ocm/extensions/accessmethods/git/method.go b/api/ocm/extensions/accessmethods/git/method.go index e8c5d927ac..59fc39e64e 100644 --- a/api/ocm/extensions/accessmethods/git/method.go +++ b/api/ocm/extensions/accessmethods/git/method.go @@ -24,6 +24,8 @@ const ( ) func init() { + // If we remove the default registration, also the docs are gone. + // so we leave the default registration in. accspeccpi.RegisterAccessType(accspeccpi.NewAccessSpecType[*AccessSpec](Type, accspeccpi.WithDescription(usage))) accspeccpi.RegisterAccessType(accspeccpi.NewAccessSpecType[*AccessSpec](TypeV1Alpha1, accspeccpi.WithFormatSpec(formatV1), accspeccpi.WithConfigHandler(ConfigHandler()))) } diff --git a/api/tech/git/fs.go b/api/tech/git/fs.go index c3bc5a84c0..bfbb4c5386 100644 --- a/api/tech/git/fs.go +++ b/api/tech/git/fs.go @@ -3,15 +3,13 @@ package git import ( "errors" "fmt" - "hash/fnv" "os" "path/filepath" + "sync" "syscall" "github.com/go-git/go-billy/v5" - "github.com/juju/fslock" "github.com/mandelsoft/vfs/pkg/memoryfs" - "github.com/mandelsoft/vfs/pkg/osfs" "github.com/mandelsoft/vfs/pkg/projectionfs" "github.com/mandelsoft/vfs/pkg/vfs" ) @@ -36,19 +34,24 @@ type fs struct { var _ billy.Filesystem = &fs{} +// file is a wrapper around a vfs.File that implements billy.File. +// it uses a mutex to lock the file, so it can be used concurrently from the same process, but +// not across processes (like a flock). type file struct { - lock *fslock.Lock vfs.File + lockMu sync.Mutex } var _ billy.File = &file{} func (f *file) Lock() error { - return f.lock.Lock() + f.lockMu.Lock() + return nil } func (f *file) Unlock() error { - return f.lock.Unlock() + f.lockMu.Unlock() + return nil } var _ billy.File = &file{} @@ -62,39 +65,9 @@ func (f *fs) Create(filename string) (billy.File, error) { } // vfsToBillyFileInfo converts a vfs.File to a billy.File -// It also creates a fslock.Lock for the file to ensure that the file is lockable -// If the vfs is an osfs.OsFs, the lock is created in the same directory as the file -// If the vfs is not an osfs.OsFs, a temporary directory is created to store the lock -// because its not trivial to store the lock for jujufs on a virtual filesystem because -// juju vfs only operates on syscalls directly and without interface abstraction its not easy to get the root. func (f *fs) vfsToBillyFileInfo(vf vfs.File) (billy.File, error) { - var lock *fslock.Lock - if f.FileSystem == osfs.OsFs { - lock = fslock.New(fmt.Sprintf("%s.lock", vf.Name())) - } else { - hash := fnv.New32() - _, _ = hash.Write([]byte(f.FileSystem.Name())) - temp, err := os.MkdirTemp("", fmt.Sprintf("git-vfs-locks-%x", hash.Sum32())) - if err != nil { - return nil, fmt.Errorf("failed to create temp dir to allow mapping vfs to git (billy) filesystem; "+ - "this temporary directory is mandatory because a virtual filesystem cannot be used to accurately depict os syslocks: %w", err) - } - _, components := vfs.Components(f.FileSystem, vf.Name()) - lockPath := filepath.Join( - temp, - filepath.Join(components[:len(components)-1]...), - fmt.Sprintf("%s.lock", components[len(components)-1]), - ) - if err := os.MkdirAll(filepath.Dir(lockPath), 0o755); err != nil { - return nil, fmt.Errorf("failed to create temp dir to allow mapping vfs to git (billy) filesystem; "+ - "this temporary directory is mandatory because a virtual filesystem cannot be used to accurately depict os syslocks: %w", err) - } - lock = fslock.New(lockPath) - } - return &file{ File: vf, - lock: lock, }, nil } diff --git a/api/tech/git/logging.go b/api/tech/git/logging.go index d5f6dfd3d0..1fce027755 100644 --- a/api/tech/git/logging.go +++ b/api/tech/git/logging.go @@ -3,5 +3,3 @@ package git import "ocm.software/ocm/api/utils/logging" var REALM = logging.DefineSubRealm("git repository", "git") - -var Log = logging.DynamicLogger(REALM) diff --git a/api/tech/git/resolver.go b/api/tech/git/resolver.go index ea1016eea2..b3aada9248 100644 --- a/api/tech/git/resolver.go +++ b/api/tech/git/resolver.go @@ -5,7 +5,6 @@ import ( "errors" "fmt" "net/http" - "os" "sync" "github.com/go-git/go-billy/v5" @@ -63,17 +62,10 @@ type Client interface { // given AuthMethod. Repository(ctx context.Context) (*git.Repository, error) - // Refresh will attempt to fetch & pull the latest changes from the remote repository. - // In case there are no changes, it will do a no-op after having realized that no changes are in the remote. - Refresh(ctx context.Context) error - - // Update will stage all changes in the repository, commit them with the given message and push them to the remote repository. - Update(ctx context.Context, msg string, push bool) error - // Setup will override the current filesystem with the given filesystem. This will be the filesystem where the repository will be stored. // There can be only one filesystem per client. // If the filesystem contains a repository already, it can be consumed by a subsequent call to Repository. - Setup(vfs.FileSystem) error + Setup(context.Context, vfs.FileSystem) error } type ClientOptions struct { @@ -88,24 +80,15 @@ type ClientOptions struct { // Commit is the commit hash to checkout after cloning the repository. // If empty, it will default to the plumbing.HEAD of the Ref. Commit string - // Author is the author to use for commits. If empty, it will default to the git config of the user running the process. - Author // AuthMethod is the authentication method to use for the repository. AuthMethod AuthMethod } -type Author struct { - Name string - Email string -} - var _ Client = &client{} func NewClient(opts ClientOptions) (Client, error) { - var pref plumbing.ReferenceName - if opts.Ref == "" { - pref = plumbing.HEAD - } else { + pref := plumbing.HEAD + if opts.Ref != "" { pref = plumbing.ReferenceName(opts.Ref) if err := pref.Validate(); err != nil { return nil, fmt.Errorf("invalid reference %q: %w", opts.Ref, err) @@ -156,7 +139,10 @@ func (c *client) Repository(ctx context.Context) (*git.Repository, error) { newRepo = true } if errors.Is(err, transport.ErrEmptyRemoteRepository) { - return git.Open(strg, billyFS) + repo, err = git.Open(strg, billyFS) + if err != nil { + return nil, fmt.Errorf("failed to open repository based on URL %q after it was determined to be an empty clone: %w", c.opts.URL, err) + } } if err != nil { @@ -168,10 +154,6 @@ func (c *client) Repository(ctx context.Context) (*git.Repository, error) { } } - if err := c.opts.applyToRepo(repo); err != nil { - return nil, err - } - c.repo = repo return repo, nil @@ -221,100 +203,10 @@ func GetStorage(base billy.Filesystem) (storage.Storer, error) { ), nil } -func (c *client) TopLevelDirs(ctx context.Context) ([]os.FileInfo, error) { - repo, err := c.Repository(ctx) - if err != nil { - return nil, err - } - - fs, err := repo.Worktree() - if err != nil { - return nil, err - } - - return fs.Filesystem.ReadDir(".") -} - -func (c *client) Refresh(ctx context.Context) error { - repo, err := c.Repository(ctx) - if err != nil { - return err - } - - worktree, err := repo.Worktree() - if err != nil { - return err - } - - if err := worktree.PullContext(ctx, &git.PullOptions{ - Auth: c.opts.AuthMethod, - RemoteName: git.DefaultRemoteName, - }); err != nil && !errors.Is(err, git.NoErrAlreadyUpToDate) && !errors.Is(err, transport.ErrEmptyRemoteRepository) { - return err - } - - return nil -} - -func (c *client) Update(ctx context.Context, msg string, push bool) error { - repo, err := c.Repository(ctx) - if err != nil { - return err - } - - worktree, err := repo.Worktree() - if err != nil { - return err - } - - if err = worktree.AddGlob("*"); err != nil { - return err - } - - _, err = worktree.Commit(msg, &git.CommitOptions{}) - - if errors.Is(err, git.ErrEmptyCommit) { - return nil - } - - if err != nil { - return err - } - - if !push { - return nil - } - - if err := repo.PushContext(ctx, &git.PushOptions{ - RemoteName: git.DefaultRemoteName, - }); err != nil && !errors.Is(err, git.NoErrAlreadyUpToDate) { - return err - } - - return nil -} - -func (c *client) Setup(system vfs.FileSystem) error { +func (c *client) Setup(ctx context.Context, system vfs.FileSystem) error { c.vfs = system - if _, err := c.Repository(context.Background()); err != nil { + if _, err := c.Repository(ctx); err != nil { return fmt.Errorf("failed to setup repository %q: %w", c.opts.URL, err) } return nil } - -func (o ClientOptions) applyToRepo(repo *git.Repository) error { - cfg, err := repo.Config() - if err != nil { - return err - } - - if o.Author.Name != "" { - cfg.User.Name = o.Author.Name - } - - if o.Author.Email != "" { - cfg.User.Email = o.Author.Email - } - - return repo.SetConfig(cfg) -} diff --git a/api/tech/git/resolver_test.go b/api/tech/git/resolver_test.go new file mode 100644 index 0000000000..d128d07808 --- /dev/null +++ b/api/tech/git/resolver_test.go @@ -0,0 +1,112 @@ +package git_test + +import ( + "embed" + "fmt" + "io" + "os" + "time" + + "github.com/go-git/go-git/v5" + "github.com/go-git/go-git/v5/plumbing" + "github.com/go-git/go-git/v5/plumbing/object" + "github.com/mandelsoft/filepath/pkg/filepath" + . "github.com/mandelsoft/goutils/testutils" + "github.com/mandelsoft/vfs/pkg/cwdfs" + "github.com/mandelsoft/vfs/pkg/osfs" + "github.com/mandelsoft/vfs/pkg/projectionfs" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "ocm.software/ocm/api/datacontext/attrs/tmpcache" + "ocm.software/ocm/api/datacontext/attrs/vfsattr" + "ocm.software/ocm/api/ocm" + self "ocm.software/ocm/api/tech/git" +) + +//go:embed testdata/repo +var testData embed.FS + +var _ = Describe("standard tests with local file repo", func() { + var ( + ctx ocm.Context + expectedBlobContent []byte + ) + + ctx = ocm.New() + + BeforeEach(func() { + tempVFS, err := cwdfs.New(osfs.New(), GinkgoT().TempDir()) + Expect(err).ToNot(HaveOccurred()) + tmpcache.Set(ctx, &tmpcache.Attribute{Path: ".", Filesystem: tempVFS}) + vfsattr.Set(ctx, tempVFS) + }) + + var repoDir string + var repoURL string + var ref string + var commit string + + BeforeEach(func() { + repoDir = GinkgoT().TempDir() + filepath.PathSeparatorString + "repo" + + repo := Must(git.PlainInit(repoDir, false)) + + repoBase := filepath.Join("testdata", "repo") + repoTestData := Must(testData.ReadDir(repoBase)) + + for _, entry := range repoTestData { + path := filepath.Join(repoBase, entry.Name()) + repoPath := filepath.Join(repoDir, entry.Name()) + + file := Must(testData.Open(path)) + + fileInRepo := Must(os.OpenFile( + repoPath, + os.O_CREATE|os.O_RDWR|os.O_TRUNC, + 0o600, + )) + + Must(io.Copy(fileInRepo, file)) + + Expect(fileInRepo.Close()).To(Succeed()) + Expect(file.Close()).To(Succeed()) + } + + wt := Must(repo.Worktree()) + Expect(wt.AddGlob("*")).To(Succeed()) + commit = Must(wt.Commit("OCM Test Commit", &git.CommitOptions{ + Author: &object.Signature{ + Name: "OCM Test", + Email: "dummy@ocm.software", + When: time.Now(), + }, + })).String() + + path := filepath.Join("testdata", "repo", "file_in_repo") + repoURL = fmt.Sprintf("file://%s", repoDir) + ref = plumbing.Master.String() + + expectedBlobContent = Must(testData.ReadFile(path)) + }) + + It("Resolver client can setup repository", func(ctx SpecContext) { + client := Must(self.NewClient(self.ClientOptions{ + URL: repoURL, + Ref: ref, + Commit: commit, + })) + + tempVFS, err := projectionfs.New(osfs.New(), GinkgoT().TempDir()) + Expect(err).ToNot(HaveOccurred()) + + Expect(client.Setup(ctx, tempVFS)).To(Succeed()) + + repo := Must(client.Repository(ctx)) + Expect(repo).ToNot(BeNil()) + + file := Must(tempVFS.Stat("file_in_repo")) + Expect(file.Size()).To(Equal(int64(len(expectedBlobContent)))) + + }) +}) diff --git a/api/tech/git/suite_test.go b/api/tech/git/suite_test.go new file mode 100644 index 0000000000..73e11ee635 --- /dev/null +++ b/api/tech/git/suite_test.go @@ -0,0 +1,13 @@ +package git_test + +import ( + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestConfig(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "OCM Git Tech Test Suite") +} diff --git a/api/tech/git/testdata/repo/file_in_repo b/api/tech/git/testdata/repo/file_in_repo new file mode 100644 index 0000000000..5eced95754 --- /dev/null +++ b/api/tech/git/testdata/repo/file_in_repo @@ -0,0 +1 @@ +Foobar \ No newline at end of file diff --git a/api/utils/accessio/downloader/git/downloader.go b/api/utils/accessio/downloader/git/downloader.go deleted file mode 100644 index 184805ba33..0000000000 --- a/api/utils/accessio/downloader/git/downloader.go +++ /dev/null @@ -1,112 +0,0 @@ -package git - -import ( - "bytes" - "context" - "errors" - "fmt" - "io" - "regexp" - "sync" - - "github.com/go-git/go-git/v5" - "github.com/go-git/go-git/v5/plumbing" - "github.com/go-git/go-git/v5/plumbing/object" - "github.com/go-git/go-git/v5/storage" - "github.com/go-git/go-git/v5/storage/memory" - - techgit "ocm.software/ocm/api/tech/git" - "ocm.software/ocm/api/utils/accessio/downloader" -) - -const localRemoteName = "origin" - -type CloseableDownloader interface { - downloader.Downloader - Close() error -} - -// Downloader simply uses the default HTTP client to download the contents of a URL. -type Downloader struct { - cloneOpts *git.CloneOptions - - matching *regexp.Regexp - - mu sync.Mutex - buf *bytes.Buffer - storage storage.Storer -} - -var _ downloader.Downloader = (*Downloader)(nil) - -func NewDownloader(url string, ref string, path string, auth techgit.AuthMethod) CloseableDownloader { - refName := plumbing.ReferenceName(ref) - return &Downloader{ - cloneOpts: &git.CloneOptions{ - Auth: auth, - URL: url, - RemoteName: localRemoteName, - ReferenceName: refName, - SingleBranch: true, - Tags: git.NoTags, - Depth: 0, - }, - matching: regexp.MustCompile(path), - buf: bytes.NewBuffer(make([]byte, 0, 4096)), - storage: memory.NewStorage(), - } -} - -func (d *Downloader) Download(w io.WriterAt) error { - d.mu.Lock() - defer d.mu.Unlock() - - ctx := context.Background() - - // no support for git archive yet, so we need to clone the repository in bare mode - repo, err := git.CloneContext(ctx, d.storage, nil, d.cloneOpts) - if err != nil { - return fmt.Errorf("failed to clone repository %s: %w", d.cloneOpts.URL, err) - } - - trees, err := repo.TreeObjects() - if err != nil { - return fmt.Errorf("failed to get tree objects: %w", err) - } - - if err := trees.ForEach(func(t *object.Tree) error { - return t.Files().ForEach(d.copyFileToBuffer) - }); err != nil { - return fmt.Errorf("failed to iterate over trees: %w", err) - } - - defer d.buf.Reset() - if _, err := w.WriteAt(d.buf.Bytes(), 0); err != nil { - return fmt.Errorf("failed to write blobs: %w", err) - } - - return nil -} - -func (d *Downloader) copyFileToBuffer(file *object.File) error { - if !d.matching.MatchString(file.Name) { - return nil - } - - reader, err := file.Reader() - if err != nil { - return err - } - _, err = io.Copy(d.buf, reader) - return errors.Join(err, reader.Close()) -} - -func (d *Downloader) Close() error { - d.mu.Lock() - defer d.mu.Unlock() - - d.buf = nil - d.storage = nil - - return nil -} diff --git a/api/utils/blobaccess/git/access.go b/api/utils/blobaccess/git/access.go index 96aa173f68..d9aba5b631 100644 --- a/api/utils/blobaccess/git/access.go +++ b/api/utils/blobaccess/git/access.go @@ -61,7 +61,7 @@ func BlobAccess(opt ...Option) (_ bpi.BlobAccess, rerr error) { }) // redirect the client to the temporary filesystem for storage of the repo, otherwise it would use memory - if err := c.Setup(repositoryFS); err != nil { + if err := c.Setup(context.Background(), repositoryFS); err != nil { return nil, err } diff --git a/api/utils/blobaccess/git/options.go b/api/utils/blobaccess/git/options.go index fa43a1ed60..44ebd7cd14 100644 --- a/api/utils/blobaccess/git/options.go +++ b/api/utils/blobaccess/git/options.go @@ -59,9 +59,6 @@ func (o *Options) ApplyTo(opts *Options) { if o.URL != "" { opts.URL = o.URL } - if o.Author.Name != "" && o.Author.Email != "" { - opts.Author = o.Author - } if o.Ref != "" { opts.Ref = o.Ref } diff --git a/go.mod b/go.mod index 9c31445818..849c503d7e 100644 --- a/go.mod +++ b/go.mod @@ -40,7 +40,6 @@ require ( github.com/golang/mock v1.6.0 github.com/google/go-github/v45 v45.2.0 github.com/hashicorp/vault-client-go v0.4.3 - github.com/juju/fslock v0.0.0-20160525022230-4d5c94c67b4b github.com/klauspost/compress v1.17.11 github.com/klauspost/pgzip v1.2.6 github.com/mandelsoft/filepath v0.0.0-20240223090642-3e2777258aa3 diff --git a/go.sum b/go.sum index a023338284..d75f2b78ba 100644 --- a/go.sum +++ b/go.sum @@ -674,8 +674,6 @@ github.com/json-iterator/go v1.1.10/go.mod h1:KdQUCv79m/52Kvf8AW2vK1V8akMuk1QjK/ github.com/json-iterator/go v1.1.12 h1:PV8peI4a0ysnczrg+LtxykD8LfKY9ML6u2jnxaEnrnM= github.com/json-iterator/go v1.1.12/go.mod h1:e30LSqwooZae/UwlEbR2852Gd8hjQvJoHmT4TnhNGBo= github.com/jtolds/gls v4.20.0+incompatible/go.mod h1:QJZ7F/aHp+rZTRtaJ1ow/lLfFfVYBRgL+9YlvaHOwJU= -github.com/juju/fslock v0.0.0-20160525022230-4d5c94c67b4b h1:FQ7+9fxhyp82ks9vAuyPzG0/vVbWwMwLJ+P6yJI5FN8= -github.com/juju/fslock v0.0.0-20160525022230-4d5c94c67b4b/go.mod h1:HMcgvsgd0Fjj4XXDkbjdmlbI505rUPBs6WBMYg2pXks= github.com/juju/loggo v0.0.0-20190526231331-6e530bcce5d8/go.mod h1:vgyd7OREkbtVEN/8IXZe5Ooef3LQePvuBm9UWj6ZL8U= github.com/julienschmidt/httprouter v1.2.0/go.mod h1:SYymIcj16QtmaHHD7aYtjjsJG7VTCxuUUipMqKk8s4w= github.com/kevinburke/ssh_config v1.2.0 h1:x584FjTGwHzMwvHx18PXxbBVzfnxogHaAReU4gf13a4=