Add more warning for service name replacement

src/pkg/cli/compose/convert.go

@@ -16,17 +16,24 @@ import (
 func ConvertServices(ctx context.Context, c client.Client, serviceConfigs compose.Services, force BuildContext) ([]*defangv1.Service, error) {
 	// Create a regexp to detect private service names in environment variable values
 	var serviceNames []string
+	var nonReplaceServiceNames []string
 	for _, svccfg := range serviceConfigs {
 		if network(&svccfg) == defangv1.Network_PRIVATE && slices.ContainsFunc(svccfg.Ports, func(p compose.ServicePortConfig) bool {
 			return p.Mode == "host" // only private services with host ports get DNS names
 		}) {
 			serviceNames = append(serviceNames, regexp.QuoteMeta(svccfg.Name))
+		} else {
+			nonReplaceServiceNames = append(nonReplaceServiceNames, regexp.QuoteMeta(svccfg.Name))
 		}
 	}
 	var serviceNameRegex *regexp.Regexp
 	if len(serviceNames) > 0 {
 		serviceNameRegex = regexp.MustCompile(`\b(?:` + strings.Join(serviceNames, "|") + `)\b`)
 	}
+	var nonReplaceServiceNameRegex *regexp.Regexp
+	if len(nonReplaceServiceNames) > 0 {
+		nonReplaceServiceNameRegex = regexp.MustCompile(`\b(?:` + strings.Join(nonReplaceServiceNames, "|") + `)\b`)
+	}
 
 	// Preload the current config so we can detect which environment variables should be passed as "secrets"
 	config, err := c.ListConfig(ctx)
@@ -130,7 +137,11 @@ func ConvertServices(ctx context.Context, c client.Client, serviceConfigs compos
 
 			// Check if the environment variable is an existing config; if so, mark it as such
 			if _, ok := slices.BinarySearch(config.Names, key); ok {
-				term.Warnf("service %q: environment variable %q overridden by config", svccfg.Name, key)
+				if serviceNameRegex != nil && serviceNameRegex.MatchString(*value) {
+					term.Warnf("service %q: environment variable %q needs service name fix-up, but is overridden by config, which will not be fixed up.", svccfg.Name, key)
+				} else {
+					term.Warnf("service %q: environment variable %q overridden by config", svccfg.Name, key)
+				}
 				envFromConfig = append(envFromConfig, key)
 				continue
 			}
@@ -142,7 +153,9 @@ func ConvertServices(ctx context.Context, c client.Client, serviceConfigs compos
 					return c.ServiceDNS(NormalizeServiceName(serviceName))
 				})
 				if val != *value {
-					term.Warnf("service %q: service names were replaced in environment variable %q: %q", svccfg.Name, key, val)
+					term.Warnf("service %q: service names were fixed up in environment variable %q: %q", svccfg.Name, key, val)
+				} else if nonReplaceServiceNameRegex != nil && nonReplaceServiceNameRegex.MatchString(*value) {
+					term.Warnf("service %q: service names in the environment variable %q were not fixed up, only services with port mode set to host will be fixed up.", svccfg.Name, key)
 				}
 			}
 			envs[key] = val

src/pkg/cli/compose/loader_test.go

@@ -45,8 +45,11 @@ func compare(actual []byte, goldenFile string) error {
 		}
 		return os.WriteFile(goldenFile, actual, 0644)
 	} else {
-		return diff(string(actual), string(golden))
+		if err := diff(string(actual), string(golden)); err != nil {
+			return fmt.Errorf("%s %w", goldenFile, err)
+		}
 	}
+	return nil
 }
 
 func diff(actualRaw, goldenRaw string) error {

src/pkg/cli/compose/validation_test.go

@@ -7,10 +7,12 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/DefangLabs/defang/src/pkg/cli/client"
 	"github.com/DefangLabs/defang/src/pkg/term"
+	defangv1 "github.com/DefangLabs/defang/src/protos/io/defang/v1"
 )
 
-func TestValidation(t *testing.T) {
+func TestValidationAndConvert(t *testing.T) {
 	oldTerm := term.DefaultTerm
 	t.Cleanup(func() {
 		term.DefaultTerm = oldTerm
@@ -30,6 +32,14 @@ func TestValidation(t *testing.T) {
 			logs.WriteString(err.Error() + "\n")
 		}
 
+		mockClient := MockClient{
+			configs: []string{"CONFIG1", "CONFIG2"},
+		}
+		if _, err = ConvertServices(context.Background(), mockClient, proj.Services, BuildContextIgnore); err != nil {
+			t.Logf("Service conversion failed: %v", err)
+			logs.WriteString(err.Error() + "\n")
+		}
+
 		// The order of the services is not guaranteed, so we sort the logs before comparing
 		logLines := strings.Split(strings.Trim(logs.String(), "\n"), "\n")
 		slices.Sort(logLines)
@@ -41,3 +51,19 @@ func TestValidation(t *testing.T) {
 		}
 	})
 }
+
+type MockClient struct {
+	client.Client
+	configs []string
+}
+
+func (m MockClient) ListConfig(ctx context.Context) (*defangv1.Secrets, error) {
+	return &defangv1.Secrets{
+		Names:   m.configs,
+		Project: "mock-project",
+	}, nil
+}
+
+func (m MockClient) ServiceDNS(name string) string {
+	return "mock-" + name
+}

src/tests/alttestproj/compose.yaml.warnings

@@ -0,0 +1 @@
+ * Compressing build context for dfnx at .

src/tests/emptyenv/compose.yaml.warnings

@@ -1,2 +1,3 @@
  ! service "emptyenv": missing compose directive: restart; assuming 'unless-stopped' (add 'restart' to silence)
- ! service "emptyenv": missing memory reservation; specify deploy.resources.reservations.memory to avoid out-of-memory errors
+ ! service "emptyenv": missing memory reservation; specify deploy.resources.reservations.memory to avoid out-of-memory errors
+ * Compressing build context for emptyenv at .

src/tests/fixupenv/compose.yaml

@@ -9,3 +9,16 @@ services:
     environment:
       - "API_URL=http://Mistral:8000"
       - "SENSITIVE_DATA"
+  bad-service:
+    image: "somedb:latest"
+    ports:
+      - mode: ingress
+        target: 5432
+  use-bad-service:
+    image: "service:latest"
+    environment:
+      - "DB_URL=bad-service:5432"
+  env-in-config:
+    image: "service:latest"
+    environment:
+      - "CONFIG1=http://Mistral:8000"

src/tests/fixupenv/compose.yaml.convert

@@ -1,4 +1,27 @@
 [
+  {
+    "name": "bad-service",
+    "image": "somedb:latest",
+    "platform": 2,
+    "internal": true,
+    "ports": [
+      {
+        "target": 5432,
+        "mode": 1
+      }
+    ],
+    "networks": 1
+  },
+  {
+    "name": "env-in-config",
+    "image": "service:latest",
+    "platform": 2,
+    "internal": true,
+    "environment": {
+      "CONFIG1": "http://mistral:8000"
+    },
+    "networks": 1
+  },
   {
     "name": "mistral",
     "image": "mistral:latest",
@@ -25,5 +48,15 @@
       }
     ],
     "networks": 1
+  },
+  {
+    "name": "use-bad-service",
+    "image": "service:latest",
+    "platform": 2,
+    "internal": true,
+    "environment": {
+      "DB_URL": "bad-service:5432"
+    },
+    "networks": 1
   }
 ]

src/tests/fixupenv/compose.yaml.golden

@@ -7,13 +7,32 @@ services:
     ports:
       - mode: host
         target: 8000
+  bad-service:
+    image: somedb:latest
+    networks:
+      default: null
+    ports:
+      - mode: ingress
+        target: 5432
+  env-in-config:
+    environment:
+      CONFIG1: http://Mistral:8000
+    image: service:latest
+    networks:
+      default: null
   ui:
     environment:
       API_URL: http://Mistral:8000
       SENSITIVE_DATA: null
     image: ui:latest
     networks:
       default: null
+  use-bad-service:
+    environment:
+      DB_URL: bad-service:5432
+    image: service:latest
+    networks:
+      default: null
 networks:
   default:
     name: fixupenv_default

src/tests/fixupenv/compose.yaml.warnings

@@ -1,5 +1,15 @@
  ! service "Mistral": missing compose directive: restart; assuming 'unless-stopped' (add 'restart' to silence)
  ! service "Mistral": missing memory reservation; specify deploy.resources.reservations.memory to avoid out-of-memory errors
+ ! service "bad-service": ingress port without healthcheck defaults to GET / HTTP/1.1
+ ! service "bad-service": missing compose directive: restart; assuming 'unless-stopped' (add 'restart' to silence)
+ ! service "bad-service": missing memory reservation; specify deploy.resources.reservations.memory to avoid out-of-memory errors
+ ! service "env-in-config": environment variable "CONFIG1" needs service name fix-up, but is overridden by config, which will not be fixed up.
+ ! service "env-in-config": missing compose directive: restart; assuming 'unless-stopped' (add 'restart' to silence)
+ ! service "env-in-config": missing memory reservation; specify deploy.resources.reservations.memory to avoid out-of-memory errors
  ! service "ui": missing compose directive: restart; assuming 'unless-stopped' (add 'restart' to silence)
  ! service "ui": missing memory reservation; specify deploy.resources.reservations.memory to avoid out-of-memory errors
+ ! service "ui": service names were fixed up in environment variable "API_URL": "http://mock-mistral:8000"
+ ! service "use-bad-service": missing compose directive: restart; assuming 'unless-stopped' (add 'restart' to silence)
+ ! service "use-bad-service": missing memory reservation; specify deploy.resources.reservations.memory to avoid out-of-memory errors
+ ! service "use-bad-service": service names in the environment variable "DB_URL" were not fixed up, only services with port mode set to host will be fixed up.
  ! service name "Mistral" was normalized to "mistral"

src/tests/redis/compose.yaml.warnings

@@ -4,4 +4,5 @@
  ! service "y": missing compose directive: restart; assuming 'unless-stopped' (add 'restart' to silence)
  ! service "y": missing memory reservation; specify deploy.resources.reservations.memory to avoid out-of-memory errors
  ! service "z": missing compose directive: restart; assuming 'unless-stopped' (add 'restart' to silence)
- ! service "z": missing memory reservation; specify deploy.resources.reservations.memory to avoid out-of-memory errors
+ ! service "z": missing memory reservation; specify deploy.resources.reservations.memory to avoid out-of-memory errors
+ ! service "z": stateful service will lose data on restart; use a managed service instead

src/tests/secretname/compose.yaml.warnings

@@ -1 +1,3 @@
- ! service "app": missing memory reservation; specify deploy.resources.reservations.memory to avoid out-of-memory errors
+ ! service "app": missing memory reservation; specify deploy.resources.reservations.memory to avoid out-of-memory errors
+ ! service "app": secrets will be exposed as environment variables, not files (use 'environment' instead)
+ * Compressing build context for app at .

src/tests/testproj/compose.yaml.warnings

@@ -1 +1,4 @@
- ! unsupported compose extension: "x-unsupported"
+ ! No port 'mode' was specified; defaulting to 'ingress' (add 'mode: ingress' to silence)
+ ! service "dfnx": secrets will be exposed as environment variables, not files (use 'environment' instead)
+ ! unsupported compose extension: "x-unsupported"
+ * Compressing build context for dfnx at .