Skip to content

Commit

Permalink
update from code reviews
Browse files Browse the repository at this point in the history
  • Loading branch information
@nullfunc
nullfunc committed Jan 2, 2025
1 parent 277f901 commit 49b645d
Show file tree
Hide file tree
Showing 5 changed files with 88 additions and 70 deletions.
8 changes: 4 additions & 4 deletions src/cmd/cli/command/cd.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ var cdDestroyCmd = &cobra.Command{
return err
}

provider, err = canIUseProvider(cmd.Context(), provider, projectName)
err = canIUseProvider(cmd.Context(), provider, projectName)
if err != nil {
return err
}
Expand All @@ -59,7 +59,7 @@ var cdDownCmd = &cobra.Command{
return err
}

provider, err = canIUseProvider(cmd.Context(), provider, projectName)
err = canIUseProvider(cmd.Context(), provider, projectName)
if err != nil {
return err
}
Expand Down Expand Up @@ -137,7 +137,7 @@ var cdListCmd = &cobra.Command{
}

if remote {
provider, err = canIUseProvider(cmd.Context(), provider, "")
err = canIUseProvider(cmd.Context(), provider, "")
if err != nil {
return err
}
Expand Down Expand Up @@ -166,7 +166,7 @@ var cdPreviewCmd = &cobra.Command{
return err
}

provider, err = canIUseProvider(cmd.Context(), provider, project.Name)
err = canIUseProvider(cmd.Context(), provider, project.Name)
if err != nil {
return err
}
Expand Down
10 changes: 5 additions & 5 deletions src/cmd/cli/command/commands.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -419,7 +419,7 @@ var whoamiCmd = &cobra.Command{
return err
}

term.Infof(str)
term.Info(str)
return nil
},
}
Expand Down Expand Up @@ -861,7 +861,7 @@ var deleteCmd = &cobra.Command{
return err
}

provider, err = canIUseProvider(cmd.Context(), provider, projectName)
err = canIUseProvider(cmd.Context(), provider, projectName)
if err != nil {
return err
}
Expand Down Expand Up @@ -1120,21 +1120,21 @@ func getProvider(ctx context.Context, loader cliClient.Loader) (cliClient.Provid
return provider, nil
}

func canIUseProvider(ctx context.Context, provider cliClient.Provider, projectName string) (cliClient.Provider, error) {
func canIUseProvider(ctx context.Context, provider cliClient.Provider, projectName string) error {
canUseReq := defangv1.CanIUseRequest{
Project: projectName,
Provider: providerID.EnumValue(),
}

resp, err := client.CanIUse(ctx, &canUseReq)
if err != nil {
return nil, err
return err
}
// Allow local override of the CD image
cdImage := pkg.Getenv("DEFANG_CD_IMAGE", resp.CdImage)
provider.SetCDImage(cdImage)

return provider, nil
return nil
}

func determineProviderID(ctx context.Context, loader cliClient.Loader) (string, error) {
Expand Down
120 changes: 67 additions & 53 deletions src/cmd/cli/command/commands_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,13 +43,15 @@ func (m *MockSsmClient) DeleteParameters(ctx context.Context, params *ssm.Delete

type mockFabricService struct {
defangv1connect.UnimplementedFabricControllerHandler
canIUseErrorText string
canIUseResponse defangv1.CanIUseResponse
canIUseIsCalled bool
canIUseError error
canIUseResponse defangv1.CanIUseResponse
}

func (m *mockFabricService) CanIUse(ctx context.Context, canUseReq *connect.Request[defangv1.CanIUseRequest]) (*connect.Response[defangv1.CanIUseResponse], error) {
if m.canIUseErrorText != "" {
return nil, connect.NewError(connect.CodeResourceExhausted, errors.New(m.canIUseErrorText))
m.canIUseIsCalled = true
if m.canIUseError != nil {
return nil, m.canIUseError
}
return connect.NewResponse(&m.canIUseResponse), nil
}
Expand Down Expand Up @@ -144,83 +146,95 @@ func TestCommandGates(t *testing.T) {
t.Fatalf("Failed to get current directory: %v", err)
}
composeDir := "../../../../src/testdata/sanity"
os.Chdir(composeDir)
if err = os.Chdir(composeDir); err != nil {
t.Fatalf("Failed to change directory: %v", err)
}

server := httptest.NewServer(handler)
t.Cleanup(func() {
os.Chdir(origDir)
server.Close()
})

type cmdPermTest struct {
name string
command []string
errorText string
wantError error
}
type cmdPermTests []cmdPermTest

t.Setenv("AWS_REGION", "us-test-2")

testData := cmdPermTests{
server := httptest.NewServer(handler)
t.Cleanup(server.Close)

testData := []struct {
name string
command []string
canIUseError error
wantError error
expectCanIUseCalled bool
}{
{
name: "compose up - aws - no access",
command: []string{"compose", "up", "--project-name=app", "--provider=aws", "--dry-run"},
errorText: "no access to use aws provider",
wantError: connect.NewError(connect.CodeResourceExhausted, errors.New("no access to use aws provider")),
name: "compose up - aws - no access",
command: []string{"compose", "up", "--provider=aws", "--dry-run"},
canIUseError: connect.NewError(connect.CodeResourceExhausted, errors.New("no access to use aws provider")),
wantError: connect.NewError(connect.CodeResourceExhausted, errors.New("no access to use aws provider")),
expectCanIUseCalled: true,
},
{
name: "compose up - defang - has access",
command: []string{"compose", "up", "--provider=defang", "--dry-run"},
errorText: "",
wantError: nil,
name: "compose up - defang - has access",
command: []string{"compose", "up", "--provider=defang", "--dry-run"},
canIUseError: nil,
wantError: nil,
expectCanIUseCalled: true,
},
{
name: "compose down - aws - no access",
command: []string{"compose", "down", "--provider=aws", "--project-name=myproj", "--dry-run"},
errorText: "no access to use aws provider",
wantError: connect.NewError(connect.CodeResourceExhausted, errors.New("no access to use aws provider")),
name: "compose down - aws - no access",
command: []string{"compose", "down", "--provider=aws", "--project-name=myproj", "--dry-run"},
canIUseError: connect.NewError(connect.CodeResourceExhausted, errors.New("no access to use aws provider")),
wantError: connect.NewError(connect.CodeResourceExhausted, errors.New("no access to use aws provider")),
expectCanIUseCalled: true,
},
{
name: "config set - aws - allowed",
command: []string{"config", "set", "var", "--project-name=app", "--provider=aws", "--dry-run"},
errorText: "no access to use aws provider",
wantError: nil,
name: "config set - aws - allowed",
command: []string{"config", "set", "var", "--project-name=app", "--provider=aws", "--dry-run"},
canIUseError: connect.NewError(connect.CodePermissionDenied, errors.New("no access to use aws provider")),
wantError: nil,
expectCanIUseCalled: false,
},
{
name: "config rm - aws - no allowed",
command: []string{"config", "rm", "var", "--project-name=app", "--provider=aws", "--dry-run"},
errorText: "no access to use aws provider",
wantError: nil,
name: "config rm - aws - not allowed",
command: []string{"config", "rm", "var", "--project-name=app", "--provider=aws", "--dry-run"},
canIUseError: connect.NewError(connect.CodePermissionDenied, errors.New("no access to use aws provider")),
wantError: nil,
expectCanIUseCalled: false,
},
{
name: "config rm - defang - allowed",
command: []string{"config", "rm", "var", "--project-name=app", "--provider=defang", "--dry-run"},
errorText: "",
wantError: nil,
name: "config rm - defang - allowed",
command: []string{"config", "rm", "var", "--project-name=app", "--provider=defang", "--dry-run"},
canIUseError: nil,
wantError: nil,
expectCanIUseCalled: false,
},
{
name: "delete service - aws - no access",
command: []string{"delete", "abc", "--provider=aws", "--dry-run"},
errorText: "no access to use aws provider",
wantError: connect.NewError(connect.CodeResourceExhausted, errors.New("no access to use aws provider")),
name: "delete service - aws - no access",
command: []string{"delete", "abc", "--provider=aws", "--dry-run"},
canIUseError: connect.NewError(connect.CodeResourceExhausted, errors.New("no access to use aws provider")),
wantError: connect.NewError(connect.CodeResourceExhausted, errors.New("no access to use aws provider")),
expectCanIUseCalled: true,
},
{
name: "whoami - allowed",
command: []string{"whoami", "--provider=aws", "--dry-run"},
errorText: "no access to use aws provider",
wantError: nil,
name: "whoami - allowed",
command: []string{"whoami", "--provider=aws", "--dry-run"},
canIUseError: connect.NewError(connect.CodePermissionDenied, errors.New("no access to use aws provider")),
wantError: nil,
expectCanIUseCalled: false,
},
}

for _, tt := range testData {
t.Run(tt.name, func(t *testing.T) {
aws.StsClient = &mockStsProviderAPI{}
pkg.SsmClientOverride = &MockSsmClient{}
mockService.canIUseErrorText = tt.errorText
mockService.canIUseIsCalled = false
mockService.canIUseError = tt.canIUseError

err := testCommand(tt.command, server.URL)

if tt.expectCanIUseCalled && !mockService.canIUseIsCalled {
t.Fatal("canIUse not called")
}

if tt.wantError == nil {
if err != nil {
if !strings.Contains(err.Error(), "dry run") && !strings.Contains(err.Error(), "no compose.yaml file found") {
Expand Down Expand Up @@ -530,7 +544,7 @@ func TestGetProvider(t *testing.T) {
t.Errorf("getProvider() failed: %v", err)
}

p, err = canIUseProvider(ctx, p, "project")
err = canIUseProvider(ctx, p, "project")
if err != nil {
t.Errorf("CanIUseProvider() failed: %v", err)
}
Expand Down Expand Up @@ -564,7 +578,7 @@ func TestGetProvider(t *testing.T) {
t.Errorf("getProvider() failed: %v", err)
}

p, err = canIUseProvider(ctx, p, "project")
err = canIUseProvider(ctx, p, "project")
if err != nil {
t.Errorf("CanIUseProvider() failed: %v", err)
}
Expand Down
4 changes: 2 additions & 2 deletions src/cmd/cli/command/compose.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ func makeComposeUpCmd() *cobra.Command {
return err
}

provider, err = canIUseProvider(cmd.Context(), provider, project.Name)
err = canIUseProvider(cmd.Context(), provider, project.Name)
if err != nil {
return err
}
Expand Down Expand Up @@ -304,7 +304,7 @@ func makeComposeDownCmd() *cobra.Command {
return err
}

provider, err = canIUseProvider(cmd.Context(), provider, projectName)
err = canIUseProvider(cmd.Context(), provider, projectName)
if err != nil {
return err
}
Expand Down
16 changes: 10 additions & 6 deletions src/pkg/cli/client/byoc/baseclient.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,14 @@ func DnsSafe(fqdn string) string {
return strings.ToLower(fqdn)
}

type ErrMultipleProjects struct {
ProjectNames []string
}

func (mp ErrMultipleProjects) Error() string {
return fmt.Sprintf("multiple projects found; use the --project-name flag to specify a project: \n%v", strings.Join(mp.ProjectNames, "\n"))
}

type ProjectBackend interface {
BootstrapList(context.Context) ([]string, error)
GetProjectUpdate(context.Context, string) (*defangv1.ProjectUpdate, error)
Expand Down Expand Up @@ -141,13 +149,9 @@ func (b *ByocBaseClient) RemoteProjectName(ctx context.Context) (string, error)
if len(projectNames) == 0 {
return "", errors.New("no projects found")
}
if len(projectNames) > 1 {
var projectList = ""
for _, projectName := range projectNames {
projectList += " - " + projectName + "\n"
}

return "", fmt.Errorf("multiple projects found; use the --project-name flag to specify a project: \n%v", projectList)
if len(projectNames) > 1 {
return "", ErrMultipleProjects{ProjectNames: projectNames}
}
term.Debug("Using default project:", projectNames[0])
return projectNames[0], nil
Expand Down

0 comments on commit 49b645d

Please sign in to comment.