Skip to content

Commit

Permalink
fix regression when compose name is specified
Browse files Browse the repository at this point in the history
  • Loading branch information
lionello committed Jun 25, 2024
1 parent f235ae5 commit dcd5e6c
Show file tree
Hide file tree
Showing 21 changed files with 117 additions and 122 deletions.
59 changes: 38 additions & 21 deletions src/pkg/cli/compose_test.go → src/pkg/cli/compose/compose_test.go
Original file line number Diff line number Diff line change
@@ -1,24 +1,21 @@
package cli
package compose

import (
"bytes"
"context"
"os"
"strings"
"testing"

"github.com/DefangLabs/defang/src/pkg/cli/compose"
"github.com/DefangLabs/defang/src/pkg/term"
"github.com/sirupsen/logrus"
)

type ComposeLoader = compose.Loader

func TestLoadCompose(t *testing.T) {
DoVerbose = true
term.SetDebug(true)
term.SetDebug(testing.Verbose())

t.Run("no project name defaults to parent directory name", func(t *testing.T) {
loader := ComposeLoader{"../../tests/noprojname/compose.yaml"}
loader := Loader{"../../../tests/noprojname/compose.yaml"}
p, err := loader.LoadCompose(context.Background())
if err != nil {
t.Fatalf("LoadCompose() failed: %v", err)
Expand All @@ -29,7 +26,7 @@ func TestLoadCompose(t *testing.T) {
})

t.Run("no project name defaults to fancy parent directory name", func(t *testing.T) {
loader := ComposeLoader{"../../tests/Fancy-Proj_Dir/compose.yaml"}
loader := Loader{"../../../tests/Fancy-Proj_Dir/compose.yaml"}
p, err := loader.LoadCompose(context.Background())
if err != nil {
t.Fatalf("LoadCompose() failed: %v", err)
Expand All @@ -40,7 +37,7 @@ func TestLoadCompose(t *testing.T) {
})

t.Run("use project name in compose file", func(t *testing.T) {
loader := ComposeLoader{"../../tests/testproj/compose.yaml"}
loader := Loader{"../../../tests/testproj/compose.yaml"}
p, err := loader.LoadCompose(context.Background())
if err != nil {
t.Fatalf("LoadCompose() failed: %v", err)
Expand All @@ -52,7 +49,7 @@ func TestLoadCompose(t *testing.T) {

t.Run("COMPOSE_PROJECT_NAME env var should override project name", func(t *testing.T) {
t.Setenv("COMPOSE_PROJECT_NAME", "overridename")
loader := ComposeLoader{"../../tests/testproj/compose.yaml"}
loader := Loader{"../../../tests/testproj/compose.yaml"}
p, err := loader.LoadCompose(context.Background())
if err != nil {
t.Fatalf("LoadCompose() failed: %v", err)
Expand All @@ -63,7 +60,7 @@ func TestLoadCompose(t *testing.T) {
})

t.Run("use project name should not be overriden by tenantID", func(t *testing.T) {
loader := ComposeLoader{"../../tests/testproj/compose.yaml"}
loader := Loader{"../../../tests/testproj/compose.yaml"}
p, err := loader.LoadCompose(context.Background())
if err != nil {
t.Fatalf("LoadCompose() failed: %v", err)
Expand All @@ -78,38 +75,38 @@ func TestLoadCompose(t *testing.T) {

// setup
setup := func() {
os.MkdirAll("../../tests/alttestproj/subdir/subdir2", 0755)
os.Chdir("../../tests/alttestproj/subdir/subdir2")
os.MkdirAll("../../../tests/alttestproj/subdir/subdir2", 0755)
os.Chdir("../../../tests/alttestproj/subdir/subdir2")
}

//teardown
teardown := func() {
os.Chdir(cwd)
os.RemoveAll("../../tests/alttestproj/subdir")
os.RemoveAll("../../../tests/alttestproj/subdir")
}

setup()
defer teardown()
t.Cleanup(teardown)

// execute test
loader := ComposeLoader{}
loader := Loader{}
p, err := loader.LoadCompose(context.Background())
if err != nil {
t.Fatalf("LoadCompose() failed: %v", err)
}
if p.Name != "tests" {
t.Errorf("LoadCompose() failed: expected project name, got %q", p.Name)
t.Errorf("LoadCompose() failed: expected project name tests, got %q", p.Name)
}
})

t.Run("load alternative compose file", func(t *testing.T) {
loader := ComposeLoader{"../../tests/alttestproj/altcomp.yaml"}
loader := Loader{"../../../tests/alttestproj/altcomp.yaml"}
p, err := loader.LoadCompose(context.Background())
if err != nil {
t.Fatalf("LoadCompose() failed: %v", err)
}
if p.Name != "tests" {
t.Errorf("LoadCompose() failed: expected project name, got %q", p.Name)
if p.Name != "altcomp" {
t.Errorf("LoadCompose() failed: expected project name altcomp, got %q", p.Name)
}
})
}
Expand All @@ -118,7 +115,7 @@ func TestComposeGoNoDoubleWarningLog(t *testing.T) {
var warnings bytes.Buffer
logrus.SetOutput(&warnings)

loader := compose.Loader{"../../tests/compose-go-warn/compose.yaml"}
loader := Loader{"../../../tests/compose-go-warn/compose.yaml"}
_, err := loader.LoadCompose(context.Background())
if err != nil {
t.Fatalf("LoadCompose() failed: %v", err)
Expand All @@ -128,3 +125,23 @@ func TestComposeGoNoDoubleWarningLog(t *testing.T) {
t.Errorf("Warning for using 'yes' for boolean from compose-go should appear exactly once")
}
}

func TestComposeOnlyOneFile(t *testing.T) {
cwd, _ := os.Getwd()
t.Cleanup(func() {
os.Chdir(cwd)
})
os.Chdir("../../../tests/toomany")

loader := Loader{}
_, err := loader.LoadCompose(context.Background())
if err == nil {
t.Fatalf("LoadCompose() failed: expected error, got nil")
}

const expected = `multiple Compose files found: ["./compose.yaml" "./docker-compose.yml"]; use -f to specify which one to use`
newCwd, _ := os.Getwd() // make the error message independent of the current working directory
if got := strings.ReplaceAll(err.Error(), newCwd, "."); got != expected {
t.Errorf("LoadCompose() failed: expected error %q, got: %s", expected, got)
}
}
29 changes: 12 additions & 17 deletions src/pkg/cli/compose/loader.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,11 @@ func (c Loader) LoadCompose(ctx context.Context) (*compose.Project, error) {
return nil, err
}
term.Debug("Loading compose file", composeFilePath)
workDir := filepath.Dir(composeFilePath)

// Compose-go uses the logrus logger, so we need to configure it to be more like our own logger
logrus.SetFormatter(&logrus.TextFormatter{DisableTimestamp: true, DisableColors: !term.StderrCanColor(), DisableLevelTruncation: true})

projOpts, err := getDefaultProjectOptions(workDir)
projOpts, err := getDefaultProjectOptions(composeFilePath)
if err != nil {
return nil, err
}
Expand All @@ -46,7 +45,7 @@ func (c Loader) LoadCompose(ctx context.Context) (*compose.Project, error) {
}

// Hack: Fill in the missing environment variables that were stripped by the normalization process
projOpts, err = getDefaultProjectOptions(workDir, cli.WithNormalization(false)) // Disable normalization to keep unset environment variables
projOpts, err = getDefaultProjectOptions(composeFilePath, cli.WithNormalization(false)) // Disable normalization to keep unset environment variables
if err != nil {
return nil, err
}
Expand All @@ -57,7 +56,7 @@ func (c Loader) LoadCompose(ctx context.Context) (*compose.Project, error) {
rawProj, err := projOpts.LoadProject(ctx)
logrus.SetOutput(currentOutput)
if err != nil {
return nil, err // there's no good reason this should fail, since we've already loaded the project
panic(err) // there's no good reason this should fail, since we've already loaded the project before
}

// TODO: Remove this hack once the PR is merged
Expand All @@ -82,7 +81,9 @@ func (c Loader) LoadCompose(ctx context.Context) (*compose.Project, error) {
return project, nil
}

func getDefaultProjectOptions(workingDir string, extraOpts ...cli.ProjectOptionsFn) (*cli.ProjectOptions, error) {
func getDefaultProjectOptions(composeFilePath string, extraOpts ...cli.ProjectOptionsFn) (*cli.ProjectOptions, error) {
workingDir := filepath.Dir(composeFilePath)

// Based on how docker compose setup its own project options
// https://github.com/docker/compose/blob/1a14fcb1e6645dd92f5a4f2da00071bd59c2e887/cmd/compose/compose.go#L326-L346
opts := []cli.ProjectOptionsFn{
Expand All @@ -96,7 +97,7 @@ func getDefaultProjectOptions(workingDir string, extraOpts ...cli.ProjectOptions
// get compose file path set by COMPOSE_FILE
cli.WithConfigFileEnv,
// if none was selected, get default compose.yaml file from current dir or parent folder
cli.WithDefaultConfigPath,
// cli.WithDefaultConfigPath, NO: this ends up picking the "first" when more than one file is found
// cli.WithName(o.ProjectName)

// Calling the 2 functions below the 2nd time as the loaded env in first call modifies the behavior of the 2nd call
Expand All @@ -110,7 +111,7 @@ func getDefaultProjectOptions(workingDir string, extraOpts ...cli.ProjectOptions
cli.WithConsistency(false), // TODO: check fails if secrets are used but top-level 'secrets:' is missing
}
opts = append(opts, extraOpts...)
projOpts, err := cli.NewProjectOptions(nil, opts...)
projOpts, err := cli.NewProjectOptions([]string{composeFilePath}, opts...)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -141,29 +142,23 @@ func getComposeFilePath(userSpecifiedComposeFile string) (string, error) {
term.Debug("Looking for compose file - searching for", searchPattern)
for {
if files, _ := filepath.Glob(filepath.Join(path, searchPattern)); len(files) > 1 {
err = fmt.Errorf("multiple Compose files found: %q; use -f to specify which one to use", files)
break
return "", fmt.Errorf("multiple Compose files found: %q; use -f to specify which one to use", files)
} else if len(files) == 1 {
// found compose file, we're done
path = files[0]
break
return files[0], nil
}

if len(userSpecifiedComposeFile) > 0 {
err = fmt.Errorf("no Compose file found at %q: %w", userSpecifiedComposeFile, os.ErrNotExist)
break
return "", fmt.Errorf("no Compose file found at %q: %w", userSpecifiedComposeFile, os.ErrNotExist)
}

// compose file not found, try parent directory
nextPath := filepath.Dir(path)
if nextPath == path {
// previous search was of root, we're done
err = types.ErrComposeFileNotFound
break
return "", types.ErrComposeFileNotFound
}

path = nextPath
}

return path, err
}
3 changes: 2 additions & 1 deletion src/pkg/cli/tail_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"time"

"github.com/DefangLabs/defang/src/pkg/cli/client"
"github.com/DefangLabs/defang/src/pkg/cli/compose"
"github.com/DefangLabs/defang/src/pkg/term"
defangv1 "github.com/DefangLabs/defang/src/protos/io/defang/v1"
"google.golang.org/protobuf/types/known/timestamppb"
Expand Down Expand Up @@ -78,7 +79,7 @@ func TestTail(t *testing.T) {
term.DefaultTerm = defaultTerm
})

proj, err := ComposeLoader{"../../tests/testproj/compose.yaml"}.LoadCompose(context.Background())
proj, err := compose.Loader{"../../tests/testproj/compose.yaml"}.LoadCompose(context.Background())
if err != nil {
t.Fatalf("LoadCompose() failed: %v", err)
}
Expand Down
2 changes: 1 addition & 1 deletion src/tests/alttestproj/altcomp.yaml
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
name: tests
name: altcomp
services:
dfnx:
restart: unless-stopped
Expand Down
38 changes: 1 addition & 37 deletions src/tests/alttestproj/compose.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -2,44 +2,8 @@ name: tests
services:
dfnx:
restart: unless-stopped
build:
context: .
dockerfile: Dockerfile
target: alttestproj
args:
DNS: dfnx
build: .
deploy:
resources:
limits:
cpus: '0.50'
memory: 512M
reservations:
cpus: '0.25'
memory: 256M
ports:
- target: 80
mode: ingress
- target: 1234
# mode: host
healthcheck:
test: ["CMD", "curl", "-f", "http://localhost/"]
# disable: true

# dfnx:
# build:
# context: .
# dockerfile: Dockerfile.dfn
# ports:
# - 80

echo:
image: ealen/echo-server
ports:
- target: 80
mode: ingress
healthcheck:
test: ["CMD", "curl", "-f", "http://localhost/"]
# domainname: echotest.gnafed.click
profiles:
- donotstart
x-defang-dns-role: arn:aws:iam::123456789012:role/ecs-service-role
27 changes: 2 additions & 25 deletions src/tests/alttestproj/compose.yaml.convert
Original file line number Diff line number Diff line change
Expand Up @@ -7,36 +7,13 @@
"replicas": 1,
"resources": {
"reservations": {
"memory": 256,
"cpus": 0.25
"memory": 256
}
}
},
"ports": [
{
"target": 80,
"mode": 1
},
{
"target": 1234,
"mode": 1
}
],
"build": {
"context": ".",
"dockerfile": "Dockerfile",
"args": {
"DNS": "dfnx"
},
"target": "alttestproj"
},
"healthcheck": {
"test": [
"CMD",
"curl",
"-f",
"http://localhost/"
]
"dockerfile": "Dockerfile"
},
"networks": 1
}
Expand Down
17 changes: 0 additions & 17 deletions src/tests/alttestproj/compose.yaml.golden
Original file line number Diff line number Diff line change
Expand Up @@ -4,29 +4,12 @@ services:
build:
context: .
dockerfile: Dockerfile
args:
DNS: dfnx
target: alttestproj
deploy:
resources:
limits:
cpus: 0.5
memory: "536870912"
reservations:
cpus: 0.25
memory: "268435456"
healthcheck:
test:
- CMD
- curl
- -f
- http://localhost/
networks:
default: null
ports:
- mode: ingress
target: 80
- target: 1234
restart: unless-stopped
networks:
default:
Expand Down
4 changes: 4 additions & 0 deletions src/tests/networks/compose.yaml
Original file line number Diff line number Diff line change
@@ -1,14 +1,18 @@
services:
service1:
image: example
networks:
invalid-network-name: {}
service2:
image: example
networks:
public: {}
service3:
image: example
networks:
- public
service4:
image: example
networks:
- private
networks:
Expand Down
Loading

0 comments on commit dcd5e6c

Please sign in to comment.