Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix regression when compose name is specified #500

Merged
merged 1 commit into from
Jun 27, 2024
Merged
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
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
Copy link
Contributor

Choose a reason for hiding this comment

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

// 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