E2E List cmd and added API calls #3

t-dedah · 2022-06-19T15:49:45Z

Aim for the PR is to get early feedback and allow Sankalp to integrate his UI side changes with delete API

Progress

Added functions for getting:-
- Cache Usage Data
- List of caches
- Delete caches
Added utils functions to:-
- Parse input to query map
- Get repo struct from repo flag and current directory
- Format cache size for printing
Completed happy path for List Command with basic stdout.

Upcoming changes

Remove additional API call logs from stdout
Improve Error Handling
Add tests

cmd/client.go

cmd/list.go

cmd/root.go

aparna-ravindra · 2022-06-20T06:02:31Z

cmd/client.go

+	ghRepo "github.com/cli/go-gh/pkg/repository"
+)
+
+type cacheInfo struct {


For now I see this is the only data model. Do we need a separate class to maintain models?

I think we should, we'd need one more wrapper model for parsing the API response as we also have total count in our list api response.

Another good yardstick would be to see if same model is being used in multiple files/packages. If yes, then it may need to moved to dedicated directory/class.

Again, this may seem redundant today but it may be important to define the layout of the project cleanly.

cmd/list.go

bishal-pdMSFT · 2022-06-20T19:14:16Z

cmd/client.go

@@ -0,0 +1,90 @@
+package cmd


Is there a specific reason to name the package as cmd. If that is to specify that this package contains comands, then client.go and utils.go should not be part of it

cmd was the default package created by cobra. We can update this to sometime like actions-cache.

Reason for keeping client.go and util.go part of the same package was thats how it was being done in the gh-s we have been trying to use as reference.

We can remove them from this package but then we will have to explicitly import them.

I would personally prefer to keep client and utils out and have cmd package only for commands code. This seems redundant today given the simplicity of the project, but once it starts growing it will get harder to manage.

cmd name seems okay as it points to "command" module if I get it right which is essentially the command handler.

bishal-pdMSFT · 2022-06-21T06:15:13Z

cmd/utils.go

@@ -0,0 +1,63 @@
+package cmd


Another similar suggestion - rather than one single utils file containing 3 different types of utils, would it be better to have a utils package which will have repoUtils, parseUtils and formatter as different files?

We will refactor and create multiple packages in the next PR. Its already in draft state

cmd/utils.go

bishal-pdMSFT · 2022-06-21T06:33:19Z

cmd/list.go

 		order, _ := cmd.Flags().GetString("order")
 		sort, _ := cmd.Flags().GetString("sort")


I guess the validation will be added later. Isn't it?

bishal-pdMSFT · 2022-06-21T06:34:06Z

cmd/list.go

@@ -21,20 +25,38 @@ var listCmd = &cobra.Command{
 	Short: "Lists the actions cache",
 	Long:  `Lists the actions cache`,
 	Run: func(cmd *cobra.Command, args []string) {
-		repo, _ := cmd.Flags().GetString("repo")
+		COMMAND = "list"
+		r, _ := cmd.Flags().GetString("repo")
 		branch, _ := cmd.Flags().GetString("branch")


The API expects the branch to be in full ref format. CLI should prepend that if user has not specified that.

cc @anuragc617 who may have better thoughts

That is being handled while creating query params in generateQueryParams

aparna-ravindra · 2022-06-21T07:02:24Z

cmd/list.go

+
+		fmt.Printf("Showing %d of %d cache entries in %s/%s\n", totalShownCacheEntry(len(caches)), len(caches), repo.Owner(), repo.Name())
+		for _, cache := range caches {
+			fmt.Printf("%s\t [%s]\t %s\t %s\n", cache.Key, formatCacheSize(cache.Size), cache.Ref, cache.LastAccessedAt)


Since we are not showing version here, there might be duplicate lines in the output here (same key and ref). Is that expected? Should it be changed?

Or: Add a TODO to display version here

This will be taken in next PR after confirmation from the designer.

cmd/utils.go

bishal-pdMSFT · 2022-06-21T13:17:43Z

cmd/root.go

@@ -6,11 +6,13 @@ import (
 	"github.com/spf13/cobra"
 )

+const VERSION = "0.0.1"


Where is this version number expected to come from? Will cli tell us which extension version is installed?

cmd/list.go

bishal-pdMSFT · 2022-06-21T13:39:25Z

cmd/list.go

+	if totalCaches < limit {
+		return totalCaches
+	}
+	return limit


use ternary operator

Golang doesnt have ternary operators.

bishal-pdMSFT · 2022-06-21T13:54:05Z

cmd/client.go

+	actionsCachesResult := apiResults["actions_caches"].([]interface{})
+
+	var caches []cacheInfo
+	for _, item := range actionsCachesResult {
+		caches = append(caches, cacheInfo{
+			Key:            item.(map[string]interface{})["key"].(string),
+			Ref:            item.(map[string]interface{})["ref"].(string),
+			LastAccessedAt: item.(map[string]interface{})["last_accessed_at"].(string),
+			Size:           item.(map[string]interface{})["size_in_bytes"].(float64),
+		})
+	}


Can we not use unmarshalling here to directly convert to the target datatype?

cc @anuragc617 @tiwarishub

And in deleteCaches as well

we are doing that in the next draft PR.

bishal-pdMSFT · 2022-06-21T13:58:10Z

@anuragc617 / @tiwarishub you may want to review this from golang best practices perspective.

t-dedah · 2022-06-22T05:58:05Z

Pending Items

Refactorization by creating separate packages
Add Models
Add Test
Reuse Sankalp's function to print cache list.
Improve Error handling

We will take this up in next PR as the initial PR was to create MVP.

Completed List cmd and added API calls

a4e8f57

t-dedah requested review from bishal-pdMSFT, aparna-ravindra and kotewar June 19, 2022 15:49

t-dedah requested a review from a team as a code owner June 19, 2022 15:49

t-dedah changed the title ~~Completed List cmd and added API calls~~ E2E List cmd and added API calls Jun 19, 2022