Updated Based on Code Review

Signed-off-by: nathannaveen <42319948+nathannaveen@users.noreply.github.com>

cmd/guaccollect/cmd/github.go

@@ -18,11 +18,12 @@ package cmd
 import (
 	"context"
 	"fmt"
-	"github.com/guacsec/guac/pkg/cli"
 	"os"
 	"strings"
 	"time"
 
+	"github.com/guacsec/guac/pkg/cli"
+
 	"github.com/guacsec/guac/internal/client/githubclient"
 	"github.com/guacsec/guac/pkg/collectsub/client"
 	csubclient "github.com/guacsec/guac/pkg/collectsub/client"
@@ -62,10 +63,8 @@ type githubOptions struct {
 var githubCmd = &cobra.Command{
 	Use:   "github <subject> is in the form of <release_url> for release mode or <owner>/<repo> for workflow mode",
 	Short: "Takes github repos and tags to download metadata documents stored in Github releases to add to GUAC graph.",
-	Long: `Takes github repos and tags to download metadata documents stored in Github releases to add to GUAC graph.
-  <github-mode> must be either "workflow", "release", or "". If "", then the default is "release".
-  if <github-mode> is "workflow", then <owner-repo> must be specified.`,
-	Args: cobra.MinimumNArgs(0),
+	Long:  `Takes github repos and tags to download metadata documents stored in Github releases to add to GUAC graph.`,
+	Args:  cobra.MinimumNArgs(0),
 	Run: func(cmd *cobra.Command, args []string) {
 		ctx := logging.WithLogger(context.Background())
 		logger := logging.FromContext(ctx)
@@ -101,7 +100,7 @@ var githubCmd = &cobra.Command{
 		collectorOpts := []github.Opt{
 			github.WithCollectDataSource(opts.dataSource),
 			github.WithClient(ghc),
-			github.WithRelease(opts.githubMode),
+			github.WithMode(opts.githubMode),
 			github.WithSbomName(opts.sbomName),
 			github.WithWorkflowName(opts.workflowFileName),
 		}
@@ -113,8 +112,12 @@ var githubCmd = &cobra.Command{
 			if !strings.Contains(opts.ownerRepoName, "/") {
 				logger.Errorf("owner-repo flag must be in the format <owner>/<repo>")
 			} else {
-				collectorOpts = append(collectorOpts, github.WithOwner(opts.ownerRepoName[:strings.Index(opts.ownerRepoName, "/")]))  // the owner name is everything before the slash
-				collectorOpts = append(collectorOpts, github.WithRepo(opts.ownerRepoName[strings.Index(opts.ownerRepoName, "/")+1:])) // the repo name is everything after the slash
+				ownerRepoName := strings.Split(opts.ownerRepoName, "/")
+				if len(ownerRepoName) != 2 {
+					logger.Errorf("owner-repo flag must be in the format <owner>/<repo>")
+				}
+				collectorOpts = append(collectorOpts, github.WithOwner(ownerRepoName[0]))
+				collectorOpts = append(collectorOpts, github.WithRepo(ownerRepoName[1]))
 			}
 		}
 
@@ -154,7 +157,7 @@ func validateGithubFlags(githubMode, sbomName, workflowFileName, natsAddr, csubA
 
 	// Otherwise direct CLI call
 
-	if githubMode == "" || githubMode == "release" {
+	if githubMode == "release" {
 		if len(args) < 1 {
 			return opts, fmt.Errorf("expected positional argument(s) for release_url(s)")
 		}
@@ -186,7 +189,7 @@ func validateGithubFlags(githubMode, sbomName, workflowFileName, natsAddr, csubA
 		opts.ownerRepoName = args[0]
 
 		if opts.ownerRepoName == "" {
-			return opts, fmt.Errorf("owner-repo flag must be in the format <owner>/<repo>")
+			return opts, fmt.Errorf("owner-repo argument must be in the format <owner>/<repo>")
 		}
 	}
 

internal/client/githubclient/githubclient.go

@@ -21,7 +21,6 @@ import (
 	"io"
 	"net/http"
 	"os"
-	"strings"
 
 	"github.com/google/go-github/v50/github"
 	"github.com/guacsec/guac/internal/client"
@@ -50,13 +49,13 @@ type GithubClient interface {
 	GetReleaseAsset(asset client.ReleaseAsset) (*client.ReleaseAssetContent, error)
 
 	// GetWorkflow fetches the workflow for a given workflow name or all workflows if the workflow name is empty
-	GetWorkflow(ctx context.Context, owner string, repo string, githubWorkflowName string) ([]client.Workflow, error)
+	GetWorkflow(ctx context.Context, owner string, repo string, githubWorkflowName string) ([]*client.Workflow, error)
 
 	// GetWorkflowRuns fetches all the workflow run for a given workflow id
 	GetWorkflowRuns(ctx context.Context, owner, repo string, workflowId int64) (*client.WorkflowRun, error)
 
 	// GetWorkflowRunArtifacts fetches all the workflow run artifacts for a given workflow run id
-	GetWorkflowRunArtifacts(ctx context.Context, owner, repo, githubSBOMName, githubWorkflowFileName string) ([]*client.WorkflowArtifactContent, error)
+	GetWorkflowRunArtifacts(ctx context.Context, owner, repo, githubSBOMName string, runID int64) ([]*client.WorkflowArtifactContent, error)
 }
 
 type githubClient struct {
@@ -122,48 +121,44 @@ func (gc *githubClient) GetLatestRelease(ctx context.Context, owner string, repo
 	return &release, nil
 }
 
-// GetWorkflow retrieves the workflow run for a specified workflow name from a given GitHub repository.
+// GetWorkflow retrieves the workflow for a specified workflow name from a given GitHub repository.
 // If the workflow name is not provided, it fetches all workflows for the repository.
 // It returns an error if the workflow name is provided but not found in the repository.
-func (gc *githubClient) GetWorkflow(ctx context.Context, owner, repo, githubWorkflowFileName string) ([]client.Workflow, error) {
+func (gc *githubClient) GetWorkflow(ctx context.Context, owner, repo, githubWorkflowFileName string) ([]*client.Workflow, error) {
+	if githubWorkflowFileName != "" {
+		workflow, _, err := gc.ghClient.Actions.GetWorkflowByFileName(ctx, owner, repo, githubWorkflowFileName)
+		if err != nil {
+			return nil, fmt.Errorf("unable to get workflow by file name: %w", err)
+		}
+
+		return []*client.Workflow{
+			{
+				Name: *workflow.Name,
+				Id:   *workflow.ID,
+			},
+		}, nil
+	}
+
 	workflows, _, err := gc.ghClient.Actions.ListWorkflows(ctx, owner, repo, nil)
 	if err != nil {
 		return nil, fmt.Errorf("unable to list workflows: %w", err)
 	}
 
-	res := []client.Workflow{}
+	var res []*client.Workflow
 
 	for _, workflow := range workflows.Workflows {
-		// The workflow file name is the last element of the path
-		splitPath := strings.Split(*workflow.Path, "/")
-		workflowFileName := splitPath[len(splitPath)-1]
-
-		if workflowFileName == githubWorkflowFileName {
-			return []client.Workflow{
-				{
-					Name: *workflow.Name,
-					Id:   *workflow.ID,
-				},
-			}, nil
-		} else if githubWorkflowFileName == "" {
-			res = append(res, client.Workflow{
-				Name: *workflow.Name,
-				Id:   *workflow.ID,
-			})
-		}
-	}
-
-	if githubWorkflowFileName != "" {
-		// if the workflow name is not empty, we should have already found a workflow and returned
-		return nil, fmt.Errorf("no workflow found with name: %s", githubWorkflowFileName)
+		res = append(res, &client.Workflow{
+			Name: *workflow.Name,
+			Id:   *workflow.ID,
+		})
 	}
 
 	return res, nil
 }
 
-// GetWorkflowRuns retrieves all the workflow runs associated with a specified workflow ID from a given GitHub repository.
+// GetLatestWorkflowRun retrieves all the workflow runs associated with a specified workflow ID from a given GitHub repository.
 // It returns an error if the workflow runs cannot be fetched.
-func (gc *githubClient) GetWorkflowRuns(ctx context.Context, owner, repo string, workflowId int64) (*client.WorkflowRun, error) {
+func (gc *githubClient) GetLatestWorkflowRun(ctx context.Context, owner, repo string, workflowId int64) (*client.WorkflowRun, error) {
 	runs, _, err := gc.ghClient.Actions.ListWorkflowRunsByID(ctx, owner, repo, workflowId, nil)
 	if err != nil {
 		return nil, fmt.Errorf("unable to list workflow runs: %w", err)
@@ -177,85 +172,69 @@ func (gc *githubClient) GetWorkflowRuns(ctx context.Context, owner, repo string,
 	return &client.WorkflowRun{WorkflowId: *runs.WorkflowRuns[0].WorkflowID, RunId: *runs.WorkflowRuns[0].ID}, nil
 }
 
-func (gc *githubClient) GetWorkflowRunArtifacts(ctx context.Context, owner, repo, githubSBOMName, githubWorkflowFileName string) ([]*client.WorkflowArtifactContent, error) {
-	var runs *github.WorkflowRuns
-	var err error
-	if githubWorkflowFileName == "" {
-		runs, _, err = gc.ghClient.Actions.ListRepositoryWorkflowRuns(ctx, owner, repo, nil)
+func (gc *githubClient) GetWorkflowRunArtifacts(ctx context.Context, owner, repo, githubSBOMName string, runID int64) ([]*client.WorkflowArtifactContent, error) {
+	var res []*client.WorkflowArtifactContent
+
+	// get workflow run artifacts
+	artifacts, _, err := gc.ghClient.Actions.ListWorkflowRunArtifacts(ctx, owner, repo, runID, nil)
+	if err != nil {
+		return nil, fmt.Errorf("unable to list workflow run artifacts: %w", err)
+	}
+	for _, j := range artifacts.Artifacts {
+		// download artifact
+		file, _, err := gc.ghClient.Actions.DownloadArtifact(ctx, owner, repo, j.GetID(), true)
 		if err != nil {
-			return nil, fmt.Errorf("unable to list workflow runs: %w", err)
+			return nil, fmt.Errorf("unable to download artifact: %w", err)
 		}
-	} else {
-		runs, _, err = gc.ghClient.Actions.ListWorkflowRunsByFileName(ctx, owner, repo, githubWorkflowFileName, nil)
+
+		// Create a new file in the local filesystem
+		out, err := os.Create(*j.Name)
 		if err != nil {
-			return nil, fmt.Errorf("unable to list workflow runs by file name: %w", err)
+			return nil, fmt.Errorf("unable to create file: %w", err)
 		}
-	}
+		defer out.Close()
 
-	var res []*client.WorkflowArtifactContent
-
-	for _, run := range runs.WorkflowRuns {
-		// get workflow run artifacts
-		artifacts, _, err := gc.ghClient.Actions.ListWorkflowRunArtifacts(ctx, owner, repo, run.GetID(), nil)
+		// Create a new HTTP client and request
+		httpClient := &http.Client{}
+		req, err := http.NewRequest("GET", file.String(), nil)
 		if err != nil {
-			return nil, fmt.Errorf("unable to list workflow run artifacts: %w", err)
+			return nil, fmt.Errorf("unable to create new request: %w", err)
 		}
-		for _, j := range artifacts.Artifacts {
-			// download artifact
-			file, _, err := gc.ghClient.Actions.DownloadArtifact(ctx, owner, repo, j.GetID(), true)
-			if err != nil {
-				return nil, fmt.Errorf("unable to download artifact: %w", err)
-			}
 
-			// Create a new file in the local filesystem
-			out, err := os.Create(*j.Name)
-			if err != nil {
-				return nil, fmt.Errorf("unable to create file: %w", err)
-			}
-			defer out.Close()
+		// Send the request
+		resp, err := httpClient.Do(req)
+		if err != nil {
+			return nil, fmt.Errorf("unable to send request: %w", err)
+		}
+		defer resp.Body.Close()
 
-			// Create a new HTTP client and request
-			httpClient := &http.Client{}
-			req, err := http.NewRequest("GET", file.String(), nil)
-			if err != nil {
-				return nil, fmt.Errorf("unable to create new request: %w", err)
-			}
+		// Write the contents of the response body to the new file
+		_, err = io.Copy(out, resp.Body)
+		if err != nil {
+			return nil, fmt.Errorf("unable to write file: %w", err)
+		}
 
-			// Send the request
-			resp, err := httpClient.Do(req)
+		// If the githubSBOMName is empty, we want to return all artifacts
+		// Otherwise, we only want to return the artifacts that have the name of githubSBOMName
+		if githubSBOMName == "" || (githubSBOMName != "" && *j.Name == githubSBOMName) {
+			// Read the contents of the file
+			fileContents, err := os.Open(*j.Name)
 			if err != nil {
-				return nil, fmt.Errorf("unable to send request: %w", err)
+				return nil, fmt.Errorf("unable to open file: %w", err)
 			}
-			defer resp.Body.Close()
+			defer fileContents.Close()
 
-			// Write the contents of the response body to the new file
-			_, err = io.Copy(out, resp.Body)
+			bytes, err := io.ReadAll(fileContents)
 			if err != nil {
-				return nil, fmt.Errorf("unable to write file: %w", err)
+				return nil, fmt.Errorf("unable to read file: %w", err)
 			}
 
-			// If the githubSBOMName is empty, we want to return all artifacts
-			// Otherwise, we only want to return the artifacts that have the name of githubSBOMName
-			if githubSBOMName == "" || (githubSBOMName != "" && *j.Name == githubSBOMName) {
-				// Read the contents of the file
-				fileContents, err := os.Open(*j.Name)
-				if err != nil {
-					return nil, fmt.Errorf("unable to open file: %w", err)
-				}
-				defer fileContents.Close()
-
-				bytes, err := io.ReadAll(fileContents)
-				if err != nil {
-					return nil, fmt.Errorf("unable to read file: %w", err)
-				}
-
-				// Write the file contents to res
-				res = append(res, &client.WorkflowArtifactContent{
-					Name:  *j.Name,
-					Bytes: bytes,
-					RunId: run.GetID(),
-				})
-			}
+			// Write the file contents to res
+			res = append(res, &client.WorkflowArtifactContent{
+				Name:  *j.Name,
+				Bytes: bytes,
+				RunId: runID,
+			})
 		}
 	}
 

internal/client/githubclient/githubclient_test.go

@@ -13,7 +13,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-//go:build integrationMerge
+//go:build integration
 
 package githubclient
 

pkg/handler/collector/github/github.go

@@ -76,6 +76,7 @@ func NewGithubCollector(opts ...Opt) (*githubCollector, error) {
 		repoToReleaseTags: map[client.Repo][]TagOrLatest{},
 		assetSuffixes:     defaultAssetSuffixes(),
 		collectDataSource: nil,
+		isRelease:         true,
 	}
 
 	for _, opt := range opts {
@@ -101,13 +102,9 @@ func WithPolling(interval time.Duration) Opt {
 	}
 }
 
-func WithRelease(githubMode string) Opt {
+func WithMode(githubMode string) Opt {
 	return func(g *githubCollector) {
-		if githubMode == "release" || githubMode == "" {
-			g.isRelease = true
-		} else {
-			g.isRelease = false // otherwise it is a workflow
-		}
+		g.isRelease = githubMode == "release"
 	}
 }
 
@@ -297,11 +294,11 @@ func (g *githubCollector) fetchWorkflowRunArtifacts(ctx context.Context, owner s
 		// get the latest workflow run
 		run, err := g.client.GetWorkflowRuns(ctx, owner, repo, workflow.Id)
 		if err != nil {
-			logger.Warnf("unable to fetch workflow runs for workflow %v: %v", workflow.Id, err)
+			logger.Errorf("unable to fetch workflow runs for workflow %v: %v", workflow.Id, err)
 			continue
 		}
 		if run == nil {
-			logger.Warnf("no workflow runs found for workflow %v", workflow.Id)
+			logger.Errorf("no workflow runs found for workflow %v", workflow.Id)
 			continue
 		}
 
@@ -310,9 +307,9 @@ func (g *githubCollector) fetchWorkflowRunArtifacts(ctx context.Context, owner s
 			continue
 		}
 
-		artifacts, err := g.client.GetWorkflowRunArtifacts(ctx, owner, repo, g.sbomName, g.workflowFileName)
+		artifacts, err := g.client.GetWorkflowRunArtifacts(ctx, owner, repo, g.sbomName, run.RunId)
 		if err != nil {
-			logger.Warnf("unable to fetch workflow run artifacts for run %v: %v", run.RunId, err)
+			logger.Errorf("unable to fetch workflow run artifacts for run %v: %v", run.RunId, err)
 			continue
 		}
 

pkg/handler/collector/github/github_test.go

@@ -13,8 +13,6 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-//go:build integration
-
 package github
 
 import (
@@ -146,15 +144,15 @@ func (m *MockGithubClient) GetReleaseAsset(asset client.ReleaseAsset) (*client.R
 	return &rac, nil
 }
 
-func (m *MockGithubClient) GetWorkflow(ctx context.Context, owner string, repo string, githubWorkflowName string) ([]client.Workflow, error) {
+func (m *MockGithubClient) GetWorkflow(ctx context.Context, owner string, repo string, githubWorkflowName string) ([]*client.Workflow, error) {
 	return nil, nil
 }
 
 func (m *MockGithubClient) GetWorkflowRuns(ctx context.Context, owner, repo string, workflowId int64) (*client.WorkflowRun, error) {
 	return nil, nil
 }
 
-func (m *MockGithubClient) GetWorkflowRunArtifacts(ctx context.Context, owner, repo, githubSBOMName, githubWorkflowName string) ([]*client.WorkflowArtifactContent, error) {
+func (m *MockGithubClient) GetWorkflowRunArtifacts(ctx context.Context, owner, repo, githubSBOMName string, runID int64) ([]*client.WorkflowArtifactContent, error) {
 	return nil, nil
 }