Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/cli/app/apply.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ func applyCommand(ctx context.Context, cmd *cobra.Command, args []string, conn *
if len(viper.GetStringSlice("file")) > 0 {
fileNames = viper.GetStringSlice("file")
}
objects, err := fileconvert.ResourcesFromPaths(cmd.Printf, fileNames...)
objects, err := fileconvert.ResourcesFromPaths(nil, cmd.Printf, fileNames...)
if err != nil {
return cli.MessageAndError("Error reading resources", err)
}
Expand Down
2 changes: 1 addition & 1 deletion cmd/cli/app/datasource/apply.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ func applyCommand(_ context.Context, cmd *cobra.Command, args []string, conn *gr
return cli.MessageAndError("Error validating file flag", err)
}

files, err := util.ExpandFileArgs(allFiles...)
files, err := util.ExpandFileArgs(nil, allFiles...)
if err != nil {
return cli.MessageAndError("Error expanding file args", err)
}
Expand Down
2 changes: 1 addition & 1 deletion cmd/cli/app/datasource/create.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ func createCommand(_ context.Context, cmd *cobra.Command, _ []string, conn *grpc
return cli.MessageAndError("Error validating file flag", err)
}

files, err := util.ExpandFileArgs(fileFlag...)
files, err := util.ExpandFileArgs(nil, fileFlag...)
if err != nil {
return cli.MessageAndError("Error expanding file args", err)
}
Expand Down
5 changes: 3 additions & 2 deletions cmd/cli/app/profile/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,13 @@ import (
"io"

"github.com/spf13/viper"
"gopkg.in/yaml.v3"

"github.com/mindersec/minder/internal/util"
"github.com/mindersec/minder/internal/util/cli"
"github.com/mindersec/minder/internal/util/cli/table"
minderv1 "github.com/mindersec/minder/pkg/api/protobuf/go/minder/v1"
"github.com/mindersec/minder/pkg/profiles"
"github.com/mindersec/minder/pkg/fileconvert"
)

// ExecOnOneProfile is a helper function to execute a function on a single profile
Expand Down Expand Up @@ -46,7 +47,7 @@ func ExecOnOneProfile(ctx context.Context, t table.Table, f string, dashOpen io.
}

func parseProfile(r io.Reader, proj string) (*minderv1.Profile, error) {
p, err := profiles.ParseYAML(r)
p, err := fileconvert.ReadResourceTyped[*minderv1.Profile](yaml.NewDecoder(r))
if err != nil {
return nil, fmt.Errorf("error reading profile from file: %w", err)
}
Expand Down
248 changes: 144 additions & 104 deletions cmd/cli/app/quickstart/quickstart.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,12 @@ package quickstart

import (
"context"
"embed"
"fmt"
"os"
"strings"

git "github.com/go-git/go-git/v5"
"github.com/go-git/go-git/v5/storage/memory"
"github.com/spf13/cobra"
"github.com/spf13/viper"
"google.golang.org/grpc"
Expand All @@ -20,13 +21,12 @@ import (

"github.com/mindersec/minder/cmd/cli/app"
"github.com/mindersec/minder/cmd/cli/app/auth"
"github.com/mindersec/minder/cmd/cli/app/profile"
minderprov "github.com/mindersec/minder/cmd/cli/app/provider"
"github.com/mindersec/minder/cmd/cli/app/repo"
internalcli "github.com/mindersec/minder/internal/cli"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not put this in internal/util/cli, which we import 2 lines later? Having two different directories for CLI-implementation-related modules is confusing to future authors.

ghclient "github.com/mindersec/minder/internal/providers/github/clients"
"github.com/mindersec/minder/internal/util/cli"
minderv1 "github.com/mindersec/minder/pkg/api/protobuf/go/minder/v1"
"github.com/mindersec/minder/pkg/profiles"
)

const (
Expand Down Expand Up @@ -128,9 +128,6 @@ In case you have registered new repositories during this flow, the profile will
`
)

//go:embed embed*
var content embed.FS

Comment on lines -131 to -133

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove the embed directory as well if we don't need this //go:embed variable?

var cmd = &cobra.Command{
Use: "quickstart",
Short: "Quickstart minder",
Expand Down Expand Up @@ -249,24 +246,35 @@ func quickstartCommand(
r := fmt.Sprintf("%s/%s", result.Owner, result.Name)
registeredRepos = append(registeredRepos, r)
}
repoURL, err := cmd.Flags().GetString("catalog-repo")
if err != nil {
return err
}
if repoURL == "" {
repoURL = defaultQuickstartCatalogRepoURL
}

return loadCatalog(cmd, ruleClient, profileClient, provider, project, registeredRepos)
}
clonedRepo, err := git.Clone(memory.NewStorage(), nil, &git.CloneOptions{
URL: repoURL,
Depth: 1,
})
if err != nil {
return fmt.Errorf("failed to load catalog repo: %w", err)
}

// loadCatalog loads and applies the quickstart rule type and profile catalog
// using the same prompts and flow as the inline implementation.
//
//nolint:gocyclo // kept as a straight extraction to preserve behavior
func loadCatalog(
cmd *cobra.Command,
ruleClient minderv1.RuleTypeServiceClient,
profileClient minderv1.ProfileServiceClient,
provider string,
project string,
registeredRepos []string,
) error {
worktree, err := clonedRepo.Worktree()
if err != nil {
return fmt.Errorf("failed to load catalog repo: %w", err)
}

catalog, err := internalcli.LoadCatalogFromFS(worktree.Filesystem, cmd.Printf)
if err != nil {
return fmt.Errorf("failed to load catalog: %w", err)
}

// Now prompt user AFTER validation
// Step 3 - Confirm rule type creation
yes := cli.PrintYesNoPrompt(cmd,
yes = cli.PrintYesNoPrompt(cmd,
stepPromptMsgRuleType,
"Proceed?",
"Quickstart operation cancelled.",
Expand All @@ -275,116 +283,147 @@ func loadCatalog(
return nil
}

// Creating the rule type
cmd.Println("Creating rule type...")

// Load the rule type from the embedded file system
reader, err := content.Open("embed/secret_scanning.yaml")
if err != nil {
return cli.MessageAndError("error opening rule type", err)
}

rt := &minderv1.RuleType{}

if err := minderv1.ParseResource(reader, rt); err != nil {
return cli.MessageAndError("error parsing rule type", err)
// Step 4 - Confirm profile creation with context
yes = cli.PrintYesNoPrompt(cmd,
fmt.Sprintf(stepPromptMsgProfile, strings.Join(registeredRepos, "\n")),
"Proceed?",
"Quickstart operation cancelled.",
true)
if !yes {
return nil
Comment on lines 277 to +293

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a little odd to have two "yes/no" prompts right in a row when nothing has happened between them. Can we coalesce them into one prompt?

}

if rt.Context == nil {
rt.Context = &minderv1.Context{}
}
// Apply all resources (transactional flow)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Include the transactional behavior in the method godoc, not at the call site.

return applyCatalog(cmd, ruleClient, profileClient, catalog, project)
}

rt.Context = &minderv1.Context{
Provider: &provider,
Project: &project,
}
const (
defaultQuickstartCatalogRepoURL = "https://github.com/mindersec/minder-rules-and-profiles"
)
Comment on lines +300 to +302

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slight preference to have constants at the top of the file, or at least before use.


// New context so we don't time out between steps
Comment thread
sachin9058 marked this conversation as resolved.
// applyCatalog creates all rule types and profiles from the catalog via gRPC services.
//
// This function is called AFTER user confirmation and validation. It creates all
// resources in the catalog. If any creation fails, the error is returned immediately.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This says "If any creation fails, the error is returned immediately", but this actually seems to try to clean up in some cases.

I'm not sure whether cleanup or "handle existing resources" is a better approach, but we should line up this doc with the "transactional flow" comment above.

// The function always prints a summary of created resources, whether new or already
// existing.
func applyCatalog(
cmd *cobra.Command,
ruleClient minderv1.RuleTypeServiceClient,
profileClient minderv1.ProfileServiceClient,
catalog *internalcli.Catalog,
project string,
) error {
ctx, cancel := getQuickstartContext(cmd.Context(), viper.GetViper())
defer cancel()

// Create the rule type in minder
_, err = ruleClient.CreateRuleType(ctx, &minderv1.CreateRuleTypeRequest{
RuleType: rt,
})
if err != nil {
if st, ok := status.FromError(err); ok {
if st.Code() != codes.AlreadyExists {
return fmt.Errorf("error creating rule type from: %w", err)
}
cmd.Println("Rule type secret_scanning already exists")
} else {
return cli.MessageAndError("error creating rule type", err)
}
projectContext := &minderv1.Context{
Project: &project,
}

// Step 4 - Confirm profile creation
yes = cli.PrintYesNoPrompt(cmd,
fmt.Sprintf(stepPromptMsgProfile, strings.Join(registeredRepos[:], "\n")),
"Proceed?",
"Quickstart operation cancelled.",
true)
if !yes {
return nil
result := applyCatalogResult{
createdRuleTypes: make([]string, 0, len(catalog.RuleTypes)),
createdProfiles: make([]string, 0, len(catalog.Profiles)),
}

// Creating the profile
cmd.Println("Creating profile...")
reader, err = content.Open("embed/profile.yaml")
if err != nil {
return cli.MessageAndError("error opening profile", err)
if err := createCatalogRuleTypes(ctx, cmd, ruleClient, catalog.RuleTypes, projectContext, &result); err != nil {
rollbackCatalogResources(ctx, ruleClient, profileClient, result)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense given two cases; for larger flows, I'd use a defer with a named err return, like so:

func DoThing() (err error) {
  created := map[string]error
  defer func() {
    if error != nil {
       for k, _ := range created {
         cleanup(k)  // not using stored error
       }
     }
  }()

  // do stuff and store in created.  Be careful not to shadow err in "if" blocks, i.e.
  err := makeStuff(created)
  if err != nil {  // using "if err :=...; err != nil" here would shadow the outer err
    return err
  }
}

return err
}

// Load the profile from the embedded file system
p, err := profiles.ParseYAML(reader)
if err != nil {
return cli.MessageAndError("error parsing profile", err)
if err := createCatalogProfiles(ctx, cmd, profileClient, catalog.Profiles, projectContext, &result); err != nil {
rollbackCatalogResources(ctx, ruleClient, profileClient, result)
return err
}

if p.Context == nil {
p.Context = &minderv1.Context{}
printCatalogSummary(cmd, result)
return nil
}

type applyCatalogResult struct {
createdRuleTypes []string
createdProfiles []string
seenExistingProfile bool
}
Comment on lines +342 to +346

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, prefer to declare structs before usage, so someone reading from the top of file can understand what's in them before they are used.


func rollbackCatalogResources(
ctx context.Context,
ruleClient minderv1.RuleTypeServiceClient,
profileClient minderv1.ProfileServiceClient,
result applyCatalogResult,
) {
for _, profileID := range result.createdProfiles {
_, _ = profileClient.DeleteProfile(ctx, &minderv1.DeleteProfileRequest{Id: profileID})
}
for _, ruleTypeID := range result.createdRuleTypes {
_, _ = ruleClient.DeleteRuleType(ctx, &minderv1.DeleteRuleTypeRequest{Id: ruleTypeID})
}
}

func createCatalogRuleTypes(
ctx context.Context,
cmd *cobra.Command,
ruleClient minderv1.RuleTypeServiceClient,
ruleTypes []*minderv1.RuleType,
projectContext *minderv1.Context,
result *applyCatalogResult,
) error {
for _, ruleType := range ruleTypes {
ruleType.Context = projectContext

p.Context = &minderv1.Context{
Provider: &provider,
Project: &project,
cmd.Printf("Creating rule type %s...\n", ruleType.GetName())
resp, err := ruleClient.CreateRuleType(ctx, &minderv1.CreateRuleTypeRequest{RuleType: ruleType})
if err != nil {
if st, ok := status.FromError(err); ok && st.Code() == codes.AlreadyExists {
cmd.Printf("Rule type %s already exists\n", ruleType.GetName())
continue
}
return fmt.Errorf("error creating rule type %s: %w", ruleType.GetName(), err)
}

name := resp.GetRuleType().GetName()
if name == "" {
name = ruleType.GetName()
}
Comment on lines +383 to +386

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slight preference for cmp.Or here, to help improve density:

Suggested change
name := resp.GetRuleType().GetName()
if name == "" {
name = ruleType.GetName()
}
name := cmp.Or(resp.GetRuleType().GetName(), ruleType.GetName())

I'm also not sure how the empty string from the server could happen. Is this something we've seen?

result.createdRuleTypes = append(result.createdRuleTypes, name)
}

// New context so we don't time out between steps
ctx, cancel = getQuickstartContext(cmd.Context(), viper.GetViper())
defer cancel()
return nil
}

alreadyExists := false
// Create the profile in minder
resp, err := profileClient.CreateProfile(ctx, &minderv1.CreateProfileRequest{
Profile: p,
})
if err != nil {
if st, ok := status.FromError(err); ok {
if st.Code() != codes.AlreadyExists {
return cli.MessageAndError("error creating profile", err)
func createCatalogProfiles(
ctx context.Context,
cmd *cobra.Command,
profileClient minderv1.ProfileServiceClient,
loadedProfiles []*minderv1.Profile,
projectContext *minderv1.Context,
result *applyCatalogResult,
) error {
for _, profileResource := range loadedProfiles {
profileResource.Context = projectContext

cmd.Printf("Creating profile %s...\n", profileResource.GetName())
resp, err := profileClient.CreateProfile(ctx, &minderv1.CreateProfileRequest{Profile: profileResource})
if err != nil {
if st, ok := status.FromError(err); ok && st.Code() == codes.AlreadyExists {
cmd.Printf("Profile %s already exists\n", profileResource.GetName())
result.seenExistingProfile = true
continue
}
alreadyExists = true
} else {
return cli.MessageAndError("error creating profile", err)
return fmt.Errorf("error creating profile %s: %w", profileResource.GetName(), err)
}

result.createdProfiles = append(result.createdProfiles, resp.GetProfile().GetId())
}

// Finish - Confirm profile creation
if alreadyExists {
// Print the "profile already exists" message
return nil
}

func printCatalogSummary(cmd *cobra.Command, result applyCatalogResult) {
if result.seenExistingProfile {
cmd.Println(cli.WarningBanner.Render(stepPromptMsgFinishExisting + stepPromptMsgFinishBase))
} else {
// Print the "profile created" message
cmd.Println(cli.WarningBanner.Render(stepPromptMsgFinishOK + stepPromptMsgFinishBase))
// Print the profile create result table
cmd.Println("Profile details (minder profile list):")
table := profile.NewProfileRulesTable(cmd.OutOrStdout())
profile.RenderProfileRulesTable(resp.GetProfile(), table)
table.Render()
}

return nil
}

func init() {
Expand All @@ -394,6 +433,7 @@ func init() {
cmd.Flags().StringP("project", "j", "", "ID of the project")
cmd.Flags().StringP("token", "t", "", "Personal Access Token (PAT) to use for enrollment")
cmd.Flags().StringP("owner", "o", "", "Owner to filter on for provider resources")
cmd.Flags().String("catalog-repo", "", "Repository URL to load quickstart catalog from")
// Bind flags
if err := viper.BindPFlag("token", cmd.Flags().Lookup("token")); err != nil {
cmd.Printf("error: %s", err)
Expand Down
2 changes: 1 addition & 1 deletion cmd/cli/app/ruletype/ruletype_apply.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ func applyCommand(cmd *cobra.Command, args []string) error {
// Combine positional args with -f flag values
allFiles := append(fileFlag, args...)

files, err := util.ExpandFileArgs(allFiles...)
files, err := util.ExpandFileArgs(nil, allFiles...)
if err != nil {
return cli.MessageAndError("Error expanding file args", err)
}
Expand Down
2 changes: 1 addition & 1 deletion cmd/cli/app/ruletype/ruletype_create.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ var createCmd = &cobra.Command{
func createCommand(cmd *cobra.Command, _ []string) error {
fileFlag, _ := cmd.Flags().GetStringArray("file")

files, err := util.ExpandFileArgs(fileFlag...)
files, err := util.ExpandFileArgs(nil, fileFlag...)
if err != nil {
return cli.MessageAndError("Error expanding file args", err)
}
Expand Down
Loading
Loading