From f808dcee623ad618be6b22d2dbf1a79fe22fe716 Mon Sep 17 00:00:00 2001 From: William Martin Date: Fri, 5 Apr 2024 14:58:12 +0200 Subject: [PATCH 1/6] Expose CacheDir on Config --- internal/config/config.go | 6 ++++++ internal/config/config_mock.go | 37 ++++++++++++++++++++++++++++++++++ internal/config/stub.go | 3 +++ 3 files changed, 46 insertions(+) diff --git a/internal/config/config.go b/internal/config/config.go index 25f3e01e9..f3ed8cf6f 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -36,6 +36,8 @@ type Config interface { Write() error Migrate(Migration) error + CacheDir() string + Aliases() *AliasConfig Authentication() *AuthConfig Browser(string) string @@ -191,6 +193,10 @@ func (c *cfg) Migrate(m Migration) error { return nil } +func (c *cfg) CacheDir() string { + return ghConfig.CacheDir() +} + func defaultFor(key string) (string, bool) { for _, co := range ConfigOptions() { if co.Key == key { diff --git a/internal/config/config_mock.go b/internal/config/config_mock.go index 586078a2c..fc25ad850 100644 --- a/internal/config/config_mock.go +++ b/internal/config/config_mock.go @@ -26,6 +26,9 @@ var _ Config = &ConfigMock{} // BrowserFunc: func(s string) string { // panic("mock out the Browser method") // }, +// CacheDirFunc: func() string { +// panic("mock out the CacheDir method") +// }, // EditorFunc: func(s string) string { // panic("mock out the Editor method") // }, @@ -72,6 +75,9 @@ type ConfigMock struct { // BrowserFunc mocks the Browser method. BrowserFunc func(s string) string + // CacheDirFunc mocks the CacheDir method. + CacheDirFunc func() string + // EditorFunc mocks the Editor method. EditorFunc func(s string) string @@ -115,6 +121,9 @@ type ConfigMock struct { // S is the s argument value. S string } + // CacheDir holds details about calls to the CacheDir method. + CacheDir []struct { + } // Editor holds details about calls to the Editor method. Editor []struct { // S is the s argument value. @@ -171,6 +180,7 @@ type ConfigMock struct { lockAliases sync.RWMutex lockAuthentication sync.RWMutex lockBrowser sync.RWMutex + lockCacheDir sync.RWMutex lockEditor sync.RWMutex lockGetOrDefault sync.RWMutex lockGitProtocol sync.RWMutex @@ -269,6 +279,33 @@ func (mock *ConfigMock) BrowserCalls() []struct { return calls } +// CacheDir calls CacheDirFunc. +func (mock *ConfigMock) CacheDir() string { + if mock.CacheDirFunc == nil { + panic("ConfigMock.CacheDirFunc: method is nil but Config.CacheDir was just called") + } + callInfo := struct { + }{} + mock.lockCacheDir.Lock() + mock.calls.CacheDir = append(mock.calls.CacheDir, callInfo) + mock.lockCacheDir.Unlock() + return mock.CacheDirFunc() +} + +// CacheDirCalls gets all the calls that were made to CacheDir. +// Check the length with: +// +// len(mockedConfig.CacheDirCalls()) +func (mock *ConfigMock) CacheDirCalls() []struct { +} { + var calls []struct { + } + mock.lockCacheDir.RLock() + calls = mock.calls.CacheDir + mock.lockCacheDir.RUnlock() + return calls +} + // Editor calls EditorFunc. func (mock *ConfigMock) Editor(s string) string { if mock.EditorFunc == nil { diff --git a/internal/config/stub.go b/internal/config/stub.go index 98c1ceaba..547ad951b 100644 --- a/internal/config/stub.go +++ b/internal/config/stub.go @@ -77,6 +77,9 @@ func NewFromString(cfgStr string) *ConfigMock { val, _ := cfg.GetOrDefault("", versionKey) return val } + mock.CacheDirFunc = func() string { + return cfg.CacheDir() + } return mock } From a89d50fc6347eaca2bf0aff0ba264e3701bb76a7 Mon Sep 17 00:00:00 2001 From: William Martin Date: Fri, 5 Apr 2024 14:59:24 +0200 Subject: [PATCH 2/6] Rework Run Log Cache so that cache dir is injected --- pkg/cmd/run/view/view.go | 76 ++++++++++++++++++++++------------- pkg/cmd/run/view/view_test.go | 4 ++ 2 files changed, 52 insertions(+), 28 deletions(-) diff --git a/pkg/cmd/run/view/view.go b/pkg/cmd/run/view/view.go index c54905422..8f6ccbc35 100644 --- a/pkg/cmd/run/view/view.go +++ b/pkg/cmd/run/view/view.go @@ -29,35 +29,48 @@ import ( ) type runLogCache interface { - Exists(string) bool - Create(string, io.Reader) error - Open(string) (*zip.ReadCloser, error) + Exists(key string) bool + Create(key string, r io.Reader) error + Open(key string) (*zip.ReadCloser, error) } -type rlc struct{} +type rlc struct { + cacheDir string +} -func (rlc) Exists(path string) bool { - if _, err := os.Stat(path); err != nil { +func (c rlc) Exists(key string) bool { + if _, err := os.Stat(c.filepath(key)); err != nil { return false } return true } -func (rlc) Create(path string, content io.Reader) error { - err := os.MkdirAll(filepath.Dir(path), 0755) - if err != nil { - return fmt.Errorf("could not create cache: %w", err) - } - out, err := os.Create(path) +func (c rlc) Create(key string, content io.Reader) error { + out, err := os.Create(c.filepath(key)) if err != nil { - return err + return fmt.Errorf("creating cache entry: %v", err) } defer out.Close() - _, err = io.Copy(out, content) - return err + + if _, err := io.Copy(out, content); err != nil { + return fmt.Errorf("writing cache entry: %v", err) + + } + + return nil } -func (rlc) Open(path string) (*zip.ReadCloser, error) { - return zip.OpenReader(path) + +func (c rlc) Open(key string) (*zip.ReadCloser, error) { + r, err := zip.OpenReader(c.filepath(key)) + if err != nil { + return nil, fmt.Errorf("opening cache entry: %v", err) + } + + return r, nil +} + +func (c rlc) filepath(key string) string { + return filepath.Join(c.cacheDir, fmt.Sprintf("run-log-%s.zip", key)) } type ViewOptions struct { @@ -85,12 +98,11 @@ type ViewOptions struct { func NewCmdView(f *cmdutil.Factory, runF func(*ViewOptions) error) *cobra.Command { opts := &ViewOptions{ - IO: f.IOStreams, - HttpClient: f.HttpClient, - Prompter: f.Prompter, - Now: time.Now, - Browser: f.Browser, - RunLogCache: rlc{}, + IO: f.IOStreams, + HttpClient: f.HttpClient, + Prompter: f.Prompter, + Now: time.Now, + Browser: f.Browser, } cmd := &cobra.Command{ @@ -126,6 +138,15 @@ func NewCmdView(f *cmdutil.Factory, runF func(*ViewOptions) error) *cobra.Comman // support `-R, --repo` override opts.BaseRepo = f.BaseRepo + config, err := f.Config() + if err != nil { + return err + } + + opts.RunLogCache = rlc{ + cacheDir: config.CacheDir(), + } + if len(args) == 0 && opts.JobID == "" { if !opts.IO.CanPrompt() { return cmdutil.FlagErrorf("run or job ID required when not running interactively") @@ -431,9 +452,8 @@ func getLog(httpClient *http.Client, logURL string) (io.ReadCloser, error) { } func getRunLog(cache runLogCache, httpClient *http.Client, repo ghrepo.Interface, run *shared.Run, attempt uint64) (*zip.ReadCloser, error) { - filename := fmt.Sprintf("run-log-%d-%d.zip", run.ID, run.StartedTime().Unix()) - filepath := filepath.Join(os.TempDir(), "gh-cli-cache", filename) - if !cache.Exists(filepath) { + cacheKey := fmt.Sprintf("%d-%d", run.ID, run.StartedTime().Unix()) + if !cache.Exists(cacheKey) { // Run log does not exist in cache so retrieve and store it logURL := fmt.Sprintf("%srepos/%s/actions/runs/%d/logs", ghinstance.RESTPrefix(repo.RepoHost()), ghrepo.FullName(repo), run.ID) @@ -461,13 +481,13 @@ func getRunLog(cache runLogCache, httpClient *http.Client, repo ghrepo.Interface return nil, err } - err = cache.Create(filepath, respReader) + err = cache.Create(cacheKey, respReader) if err != nil { return nil, err } } - return cache.Open(filepath) + return cache.Open(cacheKey) } func promptForJob(prompter shared.Prompter, cs *iostreams.ColorScheme, jobs []shared.Job) (*shared.Job, error) { diff --git a/pkg/cmd/run/view/view_test.go b/pkg/cmd/run/view/view_test.go index 1135212be..28a63d4d2 100644 --- a/pkg/cmd/run/view/view_test.go +++ b/pkg/cmd/run/view/view_test.go @@ -12,6 +12,7 @@ import ( "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/internal/browser" + "github.com/cli/cli/v2/internal/config" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/prompter" "github.com/cli/cli/v2/pkg/cmd/run/shared" @@ -137,6 +138,9 @@ func TestNewCmdView(t *testing.T) { f := &cmdutil.Factory{ IOStreams: ios, + Config: func() (config.Config, error) { + return config.NewBlankConfig(), nil + }, } argv, err := shlex.Split(tt.cli) From e644dc50d6bd60cd605ebf3beb9a3b80098a635c Mon Sep 17 00:00:00 2001 From: William Martin Date: Fri, 5 Apr 2024 15:16:12 +0200 Subject: [PATCH 3/6] Capture error on Run Log Cache Exists --- pkg/cmd/run/view/view.go | 23 +++++++++++++++++------ pkg/cmd/run/view/view_test.go | 4 ++-- 2 files changed, 19 insertions(+), 8 deletions(-) diff --git a/pkg/cmd/run/view/view.go b/pkg/cmd/run/view/view.go index 8f6ccbc35..ea285c8ea 100644 --- a/pkg/cmd/run/view/view.go +++ b/pkg/cmd/run/view/view.go @@ -29,7 +29,7 @@ import ( ) type runLogCache interface { - Exists(key string) bool + Exists(key string) (bool, error) Create(key string, r io.Reader) error Open(key string) (*zip.ReadCloser, error) } @@ -38,11 +38,17 @@ type rlc struct { cacheDir string } -func (c rlc) Exists(key string) bool { - if _, err := os.Stat(c.filepath(key)); err != nil { - return false +func (c rlc) Exists(key string) (bool, error) { + _, err := os.Stat(c.filepath(key)) + if err == nil { + return true, nil } - return true + + if errors.Is(err, os.ErrNotExist) { + return false, nil + } + + return false, fmt.Errorf("checking cache entry: %v", err) } func (c rlc) Create(key string, content io.Reader) error { @@ -453,7 +459,12 @@ func getLog(httpClient *http.Client, logURL string) (io.ReadCloser, error) { func getRunLog(cache runLogCache, httpClient *http.Client, repo ghrepo.Interface, run *shared.Run, attempt uint64) (*zip.ReadCloser, error) { cacheKey := fmt.Sprintf("%d-%d", run.ID, run.StartedTime().Unix()) - if !cache.Exists(cacheKey) { + isCached, err := cache.Exists(cacheKey) + if err != nil { + return nil, err + } + + if !isCached { // Run log does not exist in cache so retrieve and store it logURL := fmt.Sprintf("%srepos/%s/actions/runs/%d/logs", ghinstance.RESTPrefix(repo.RepoHost()), ghrepo.FullName(repo), run.ID) diff --git a/pkg/cmd/run/view/view_test.go b/pkg/cmd/run/view/view_test.go index 28a63d4d2..299595509 100644 --- a/pkg/cmd/run/view/view_test.go +++ b/pkg/cmd/run/view/view_test.go @@ -1495,8 +1495,8 @@ func Test_attachRunLog(t *testing.T) { type testRunLogCache struct{} -func (testRunLogCache) Exists(path string) bool { - return false +func (testRunLogCache) Exists(path string) (bool, error) { + return false, nil } func (testRunLogCache) Create(path string, content io.Reader) error { return nil From a3ffc1ca33fe742ab5d5e9c31078cc7b7a80dcfb Mon Sep 17 00:00:00 2001 From: William Martin Date: Fri, 5 Apr 2024 15:18:18 +0200 Subject: [PATCH 4/6] Use real Run Log Cache in run view tests --- pkg/cmd/run/view/view_test.go | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/pkg/cmd/run/view/view_test.go b/pkg/cmd/run/view/view_test.go index 299595509..56911e6c9 100644 --- a/pkg/cmd/run/view/view_test.go +++ b/pkg/cmd/run/view/view_test.go @@ -1346,7 +1346,9 @@ func TestViewRun(t *testing.T) { browser := &browser.Stub{} tt.opts.Browser = browser - rlc := testRunLogCache{} + rlc := rlc{ + cacheDir: t.TempDir(), + } tt.opts.RunLogCache = rlc t.Run(tt.name, func(t *testing.T) { @@ -1493,18 +1495,6 @@ func Test_attachRunLog(t *testing.T) { } } -type testRunLogCache struct{} - -func (testRunLogCache) Exists(path string) (bool, error) { - return false, nil -} -func (testRunLogCache) Create(path string, content io.Reader) error { - return nil -} -func (testRunLogCache) Open(path string) (*zip.ReadCloser, error) { - return zip.OpenReader("./fixtures/run_log.zip") -} - var barfTheFobLogOutput = heredoc.Doc(` cool job barz the fob log line 1 cool job barz the fob log line 2 From 103586a94c89febf3537b23f73054d4c59b7d466 Mon Sep 17 00:00:00 2001 From: William Martin Date: Fri, 5 Apr 2024 15:33:42 +0200 Subject: [PATCH 5/6] Remove RunLogCache interface --- pkg/cmd/run/view/view.go | 22 ++++++++-------------- pkg/cmd/run/view/view_test.go | 3 +-- 2 files changed, 9 insertions(+), 16 deletions(-) diff --git a/pkg/cmd/run/view/view.go b/pkg/cmd/run/view/view.go index ea285c8ea..0b311a66a 100644 --- a/pkg/cmd/run/view/view.go +++ b/pkg/cmd/run/view/view.go @@ -28,17 +28,11 @@ import ( "github.com/spf13/cobra" ) -type runLogCache interface { - Exists(key string) (bool, error) - Create(key string, r io.Reader) error - Open(key string) (*zip.ReadCloser, error) -} - -type rlc struct { +type RunLogCache struct { cacheDir string } -func (c rlc) Exists(key string) (bool, error) { +func (c RunLogCache) Exists(key string) (bool, error) { _, err := os.Stat(c.filepath(key)) if err == nil { return true, nil @@ -51,7 +45,7 @@ func (c rlc) Exists(key string) (bool, error) { return false, fmt.Errorf("checking cache entry: %v", err) } -func (c rlc) Create(key string, content io.Reader) error { +func (c RunLogCache) Create(key string, content io.Reader) error { out, err := os.Create(c.filepath(key)) if err != nil { return fmt.Errorf("creating cache entry: %v", err) @@ -66,7 +60,7 @@ func (c rlc) Create(key string, content io.Reader) error { return nil } -func (c rlc) Open(key string) (*zip.ReadCloser, error) { +func (c RunLogCache) Open(key string) (*zip.ReadCloser, error) { r, err := zip.OpenReader(c.filepath(key)) if err != nil { return nil, fmt.Errorf("opening cache entry: %v", err) @@ -75,7 +69,7 @@ func (c rlc) Open(key string) (*zip.ReadCloser, error) { return r, nil } -func (c rlc) filepath(key string) string { +func (c RunLogCache) filepath(key string) string { return filepath.Join(c.cacheDir, fmt.Sprintf("run-log-%s.zip", key)) } @@ -85,7 +79,7 @@ type ViewOptions struct { BaseRepo func() (ghrepo.Interface, error) Browser browser.Browser Prompter shared.Prompter - RunLogCache runLogCache + RunLogCache RunLogCache RunID string JobID string @@ -149,7 +143,7 @@ func NewCmdView(f *cmdutil.Factory, runF func(*ViewOptions) error) *cobra.Comman return err } - opts.RunLogCache = rlc{ + opts.RunLogCache = RunLogCache{ cacheDir: config.CacheDir(), } @@ -457,7 +451,7 @@ func getLog(httpClient *http.Client, logURL string) (io.ReadCloser, error) { return resp.Body, nil } -func getRunLog(cache runLogCache, httpClient *http.Client, repo ghrepo.Interface, run *shared.Run, attempt uint64) (*zip.ReadCloser, error) { +func getRunLog(cache RunLogCache, httpClient *http.Client, repo ghrepo.Interface, run *shared.Run, attempt uint64) (*zip.ReadCloser, error) { cacheKey := fmt.Sprintf("%d-%d", run.ID, run.StartedTime().Unix()) isCached, err := cache.Exists(cacheKey) if err != nil { diff --git a/pkg/cmd/run/view/view_test.go b/pkg/cmd/run/view/view_test.go index 56911e6c9..f67f7e612 100644 --- a/pkg/cmd/run/view/view_test.go +++ b/pkg/cmd/run/view/view_test.go @@ -1346,10 +1346,9 @@ func TestViewRun(t *testing.T) { browser := &browser.Stub{} tt.opts.Browser = browser - rlc := rlc{ + tt.opts.RunLogCache = RunLogCache{ cacheDir: t.TempDir(), } - tt.opts.RunLogCache = rlc t.Run(tt.name, func(t *testing.T) { err := runView(tt.opts) From c2aee1e402fbcaa042b7981abf950022e72831b6 Mon Sep 17 00:00:00 2001 From: William Martin Date: Fri, 5 Apr 2024 15:39:33 +0200 Subject: [PATCH 6/6] Ensure cache dir is always available in RunLogCache --- pkg/cmd/run/view/view.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pkg/cmd/run/view/view.go b/pkg/cmd/run/view/view.go index 0b311a66a..f79702181 100644 --- a/pkg/cmd/run/view/view.go +++ b/pkg/cmd/run/view/view.go @@ -46,6 +46,10 @@ func (c RunLogCache) Exists(key string) (bool, error) { } func (c RunLogCache) Create(key string, content io.Reader) error { + if err := os.MkdirAll(filepath.Dir(c.cacheDir), 0755); err != nil { + return fmt.Errorf("creating cache directory: %v", err) + } + out, err := os.Create(c.filepath(key)) if err != nil { return fmt.Errorf("creating cache entry: %v", err)