From 8365ebe6190a3634be1aee944a0d3c76316e28bf Mon Sep 17 00:00:00 2001 From: Sankalp Kotewar <98868223+kotewar@users.noreply.github.com> Date: Mon, 18 Jul 2022 14:51:28 +0530 Subject: [PATCH] Bugbash fixes and refactoring (#14) * Completed List cmd and added API calls * Minor comments and add delete code to pass linting * Typo in descriptions * Minor comments * Validations * Validations-1 * improved branch flag validation * removed build * working after refactory with bad names * Command working, test not working * Corrected creation of service * Finalized structure using service * Deleted tests * cleanup * cleanup * cleanup * removed space with tab * aligned types in model.go * Update model.go * resolved comments * Refactor * removed long descriptions * Working incomplete tests * Completed tests * cleanup * checks * PR comments * PR comments * minor comment issue * minor comment issue * Added test cases for Delete * updated tests to work with workflow * Added missing error condition * Updated tests to support new option service * Improved eror handling for list * Fixed test case * Improved error handling * Error handling and test cases for delete API calls * Added test case for user confirmation delete. * Removed unused import from test * Fixed test case for error scenario * Upgraded go-gh * reusing rest client error * Fix for failing windows test cases * help cmd removed when cache isnt present on delete * Pretty print ratio and space between cols modified * Error handling wrapping * Reverted back error message after silencing help * Bugbash fixes for int limit, zero cache list msg * Test case fixes for error message changes * Handling no cache list scenario with(out) key * Minor Refactoring and avoided code duplication * Removed unused inputs after resolving conflicts * Formatted test file * removed err5xx as they all have same value. * Removed err5xx from list as well * Help and error message enhancements for list. * changing commandname to avoid conflicts * Ran `go fmt` formatter against all .go files * Removed command from root.go * Updated version to 1.0.0 Co-authored-by: t-dedah Co-authored-by: Deepak Dahiya <59823596+t-dedah@users.noreply.github.com> --- cmd/delete.go | 24 ++++-------------------- cmd/list.go | 32 +++++++++++++------------------- cmd/list_test.go | 10 ++++++---- cmd/root.go | 4 +--- internal/test_utils.go | 2 +- internal/utils.go | 19 ++++++++++++++++--- internal/utils_test.go | 2 +- service/actions_cache_test.go | 2 +- types/errors.go | 3 +-- types/options.go | 2 +- 10 files changed, 45 insertions(+), 55 deletions(-) diff --git a/cmd/delete.go b/cmd/delete.go index 0c6b6a3..43041bd 100644 --- a/cmd/delete.go +++ b/cmd/delete.go @@ -1,7 +1,6 @@ package cmd import ( - "errors" "fmt" "net/url" "strings" @@ -10,14 +9,13 @@ import ( "github.com/actions/gh-actions-cache/internal" "github.com/actions/gh-actions-cache/service" "github.com/actions/gh-actions-cache/types" - "github.com/cli/go-gh/pkg/api" "github.com/spf13/cobra" ) var choice string = "" func NewCmdDelete() *cobra.Command { - COMMAND = "delete" + deleteCommand := "delete" f := types.DeleteOptions{} var deleteCmd = &cobra.Command{ @@ -38,7 +36,7 @@ func NewCmdDelete() *cobra.Command { // This will silence the usage (help) message as they are not needed for errors beyond this point cmd.SilenceUsage = true - artifactCache, err := service.NewArtifactCache(repo, COMMAND, VERSION) + artifactCache, err := service.NewArtifactCache(repo, deleteCommand, VERSION) if err != nil { return types.HandledError{Message: err.Error(), InnerError: err} } @@ -49,14 +47,7 @@ func NewCmdDelete() *cobra.Command { if !f.Confirm { matchedCaches, err := getCacheListWithExactMatch(f, artifactCache) if err != nil { - var httpError api.HTTPError - if errors.As(err, &httpError) && httpError.StatusCode == 404 { - return types.HandledError{Message: "The given repo does not exist.", InnerError: err} - } else if errors.As(err, &httpError) && httpError.StatusCode >= 400 && httpError.StatusCode < 500 { - return types.HandledError{Message: httpError.Message, InnerError: err} - } else { - return types.HandledError{Message: "We could not process your request due to internal error.", InnerError: err} - } + return internal.HttpErrorHandler(err, "The given repo does not exist.") } matchedCachesLen := len(matchedCaches) if matchedCachesLen == 0 { @@ -81,14 +72,7 @@ func NewCmdDelete() *cobra.Command { if f.Confirm { cachesDeleted, err := artifactCache.DeleteCaches(queryParams) if err != nil { - var httpError api.HTTPError - if errors.As(err, &httpError) && httpError.StatusCode == 404 { - return types.HandledError{Message: fmt.Sprintf("Cache with input key '%s' does not exist", f.Key), InnerError: err} - } else if errors.As(err, &httpError) && httpError.StatusCode >= 400 && httpError.StatusCode < 500 { - return types.HandledError{Message: httpError.Message, InnerError: err} - } else { - return types.HandledError{Message: "We could not process your request due to internal error.", InnerError: err} - } + return internal.HttpErrorHandler(err, fmt.Sprintf("Cache with input key '%s' does not exist", f.Key)) } if cachesDeleted > 0 { diff --git a/cmd/list.go b/cmd/list.go index fabfa69..6a9c3dd 100644 --- a/cmd/list.go +++ b/cmd/list.go @@ -1,23 +1,21 @@ package cmd import ( - "errors" "fmt" "net/url" "github.com/actions/gh-actions-cache/internal" "github.com/actions/gh-actions-cache/service" "github.com/actions/gh-actions-cache/types" - "github.com/cli/go-gh/pkg/api" "github.com/spf13/cobra" ) func NewCmdList() *cobra.Command { - COMMAND = "list" + listCommand := "list" f := types.ListOptions{} - var listCmd = &cobra.Command { + var listCmd = &cobra.Command{ Use: "list", Short: "Lists the actions cache", RunE: func(cmd *cobra.Command, args []string) error { @@ -38,14 +36,14 @@ func NewCmdList() *cobra.Command { return err } - artifactCache, err := service.NewArtifactCache(repo, COMMAND, VERSION) + artifactCache, err := service.NewArtifactCache(repo, listCommand, VERSION) if err != nil { return types.HandledError{Message: err.Error(), InnerError: err} } if f.Branch == "" && f.Key == "" { totalCacheSize, err := artifactCache.GetCacheUsage() - if err == nil { + if err == nil && totalCacheSize > 0 { fmt.Printf("Total caches size %s\n\n", internal.FormatCacheSize(totalCacheSize)) } } @@ -54,28 +52,24 @@ func NewCmdList() *cobra.Command { f.GenerateQueryParams(queryParams) listCacheResponse, err := artifactCache.ListCaches(queryParams) if err != nil { - var httpError api.HTTPError - if errors.As(err, &httpError) && httpError.StatusCode == 404 { - return types.HandledError{Message: "The given repo does not exist.", InnerError: err} - } else if errors.As(err, &httpError) && httpError.StatusCode >= 400 && httpError.StatusCode < 500 { - return types.HandledError{Message: httpError.Message, InnerError: err} - } else { - return types.HandledError{Message: "We could not process your request due to internal error.", InnerError: err} - } + return internal.HttpErrorHandler(err, "The given repo does not exist.") } totalCaches := listCacheResponse.TotalCount caches := listCacheResponse.ActionsCaches - - fmt.Printf("Showing %d of %d cache entries in %s/%s\n\n", displayedEntriesCount(len(caches), f.Limit), totalCaches, repo.Owner(), repo.Name()) - internal.PrettyPrintCacheList(caches) - return nil + if len(caches) > 0 { + fmt.Printf("Showing %d of %d cache entries in %s/%s\n\n", displayedEntriesCount(len(caches), f.Limit), totalCaches, repo.Owner(), repo.Name()) + internal.PrettyPrintCacheList(caches) + } else { + fmt.Printf("There are no Actions caches currently present in this repo or for the provided filters\n") + } + return nil }, } listCmd.Flags().StringVarP(&f.Repo, "repo", "R", "", "Select another repository for finding actions cache.") listCmd.Flags().StringVarP(&f.Branch, "branch", "B", "", "Filter by branch") - listCmd.Flags().IntVarP(&f.Limit, "limit", "", 30, "Maximum number of items to fetch (default is 30, max limit is 100)") + listCmd.Flags().IntVarP(&f.Limit, "limit", "", 30, "Number of items to fetch between 1 and 100") listCmd.Flags().StringVarP(&f.Key, "key", "", "", "Filter by key") listCmd.Flags().StringVarP(&f.Order, "order", "", "", "Order of caches returned (asc/desc)") listCmd.Flags().StringVarP(&f.Sort, "sort", "", "", "Sort fetched caches (last-used/size/created-at)") diff --git a/cmd/list_test.go b/cmd/list_test.go index 66475e1..419425f 100644 --- a/cmd/list_test.go +++ b/cmd/list_test.go @@ -3,12 +3,14 @@ package cmd import ( "errors" "fmt" - "testing" + "reflect" + "testing" "github.com/actions/gh-actions-cache/internal" - "github.com/stretchr/testify/assert" "github.com/actions/gh-actions-cache/types" + "github.com/stretchr/testify/assert" + "gopkg.in/h2non/gock.v1" ) @@ -44,7 +46,7 @@ func TestListWithNegativeLimit(t *testing.T) { err := cmd.Execute() assert.NotNil(t, err) - assert.Equal(t, err, fmt.Errorf("-1 is not a valid value for limit flag. Allowed values: 1-100")) + assert.Equal(t, err, fmt.Errorf("-1 is not a valid integer value for limit flag. Allowed values: 1-100")) assert.True(t, gock.IsDone(), internal.PrintPendingMocks(gock.Pending())) } @@ -56,7 +58,7 @@ func TestListWithIncorrectLimit(t *testing.T) { err := cmd.Execute() assert.NotNil(t, err) - assert.Equal(t, err, fmt.Errorf("101 is not a valid value for limit flag. Allowed values: 1-100")) + assert.Equal(t, err, fmt.Errorf("101 is not a valid integer value for limit flag. Allowed values: 1-100")) assert.True(t, gock.IsDone(), internal.PrintPendingMocks(gock.Pending())) } diff --git a/cmd/root.go b/cmd/root.go index 77c9cee..cb879ba 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -6,9 +6,7 @@ import ( "github.com/spf13/cobra" ) -const VERSION = "0.0.1" - -var COMMAND string = "" +const VERSION = "1.0.0" var rootCmd = &cobra.Command{ Use: "gh-actions-cache", diff --git a/internal/test_utils.go b/internal/test_utils.go index bbdaaef..b4521ee 100644 --- a/internal/test_utils.go +++ b/internal/test_utils.go @@ -13,4 +13,4 @@ func PrintPendingMocks(mocks []gock.Mock) string { paths = append(paths, mock.Request().URLStruct.String()) } return fmt.Sprintf("%d unmatched mocks: %s", len(paths), strings.Join(paths, ", ")) -} \ No newline at end of file +} diff --git a/internal/utils.go b/internal/utils.go index 0896c94..f572cec 100644 --- a/internal/utils.go +++ b/internal/utils.go @@ -1,6 +1,7 @@ package internal import ( + "errors" "fmt" "math" "os" @@ -12,6 +13,7 @@ import ( "github.com/TwiN/go-color" "github.com/actions/gh-actions-cache/types" gh "github.com/cli/go-gh" + "github.com/cli/go-gh/pkg/api" ghRepo "github.com/cli/go-gh/pkg/repository" "github.com/cli/safeexec" "github.com/mattn/go-isatty" @@ -51,17 +53,17 @@ func FormatCacheSize(size_in_bytes float64) string { func PrettyPrintCacheList(caches []types.ActionsCache) { width, _, _ := getTerminalWidth(os.Stdout) width = int(math.Max(float64(width), 100)) - sizeWidth := SIZE_COLUMN_WIDTH // hard-coded size as the content is scoped timeWidth := LAST_ACCESSED_AT_COLUMN_WIDTH // hard-coded size as the content is scoped - keyWidth := int(math.Floor(0.65 * float64(width-15-20))) - refWidth := int(math.Floor(0.20 * float64(width-15-20))) + keyWidth := int(math.Floor(0.65 * float64(width-SIZE_COLUMN_WIDTH-LAST_ACCESSED_AT_COLUMN_WIDTH))) + refWidth := int(math.Floor(0.20 * float64(width-SIZE_COLUMN_WIDTH-LAST_ACCESSED_AT_COLUMN_WIDTH))) for _, cache := range caches { var formattedRow string = getFormattedCacheInfo(cache, keyWidth, sizeWidth, refWidth, timeWidth) fmt.Println(formattedRow) } } + func PrettyPrintTrimmedCacheList(caches []types.ActionsCache) { length := len(caches) limit := 30 @@ -127,3 +129,14 @@ func getTerminalWidth(f *os.File) (int, int, error) { } return 100, 100, nil } + +func HttpErrorHandler(err error, errMsg404 string) types.HandledError { + var httpError api.HTTPError + if errors.As(err, &httpError) && httpError.StatusCode == 404 { + return types.HandledError{Message: errMsg404, InnerError: err} + } else if errors.As(err, &httpError) && httpError.StatusCode >= 400 && httpError.StatusCode < 500 { + return types.HandledError{Message: httpError.Message, InnerError: err} + } else { + return types.HandledError{Message: "We could not process your request due to internal error.", InnerError: err} + } +} diff --git a/internal/utils_test.go b/internal/utils_test.go index d2e2717..1bf6e68 100644 --- a/internal/utils_test.go +++ b/internal/utils_test.go @@ -52,4 +52,4 @@ func TestFormatCacheSize_GB(t *testing.T) { assert.NotNil(t, cacheSizeDetailString) assert.Equal(t, cacheSizeDetailString, "1.50 GB") -} \ No newline at end of file +} diff --git a/service/actions_cache_test.go b/service/actions_cache_test.go index fba2c85..c89ac8f 100644 --- a/service/actions_cache_test.go +++ b/service/actions_cache_test.go @@ -12,7 +12,7 @@ import ( "gopkg.in/h2non/gock.v1" ) -const VERSION string = "0.0.1" +const VERSION string = "1.0.0" func TestGetCacheUsage_CorrectRepo(t *testing.T) { t.Cleanup(gock.Off) diff --git a/types/errors.go b/types/errors.go index 634e151..c95f2bc 100644 --- a/types/errors.go +++ b/types/errors.go @@ -5,7 +5,7 @@ import ( ) type HandledError struct { - Message string + Message string InnerError error } @@ -13,4 +13,3 @@ type HandledError struct { func (err HandledError) Error() string { return fmt.Sprintf(err.Message) } - diff --git a/types/options.go b/types/options.go index 8e90472..b1b05ff 100644 --- a/types/options.go +++ b/types/options.go @@ -41,7 +41,7 @@ func (o *ListOptions) Validate() error { } if o.Limit < 1 || o.Limit > 100 { - return fmt.Errorf(fmt.Sprintf("%d is not a valid value for limit flag. Allowed values: 1-100", o.Limit)) + return fmt.Errorf(fmt.Sprintf("%d is not a valid integer value for limit flag. Allowed values: 1-100", o.Limit)) } return nil