logging changes (#1049)

* logging changes

* group service env logs to one line

* update warnings

* expanded some log lines

* revert to use original devcontainer config

* remove underscore replacement

* Apply suggestions from code review

Co-authored-by: Jordan Stephens <[email protected]>

* review changes

---------

Co-authored-by: Jordan Stephens <[email protected]>

Author: nullfunc

src/cmd/cli/command/commands.go

@@ -600,7 +600,15 @@ var generateCmd = &cobra.Command{
 
 		// Check if the current folder is empty
 		if empty, err := pkg.IsDirEmpty(prompt.Folder); !os.IsNotExist(err) && !empty {
-			term.Warnf("The folder %q is not empty. We recommend running this command in an empty folder.", prompt.Folder)
+			nonEmptyFolder := fmt.Sprintf("The folder %q is not empty. We recommend running this command in an empty folder.", prompt.Folder)
+
+			var confirm bool
+			err := survey.AskOne(&survey.Confirm{
+				Message: nonEmptyFolder + " Continue creating project?",
+			}, &confirm, survey.WithStdio(term.DefaultTerm.Stdio()))
+			if err == nil && !confirm {
+				os.Exit(1)
+			}
 		}
 
 		if sample != "" {
@@ -1166,6 +1174,10 @@ func getProvider(ctx context.Context, loader cliClient.Loader) (cliClient.Provid
 		if !doInEnv() {
 			term.Warn("DigitalOcean provider was selected, but DIGITALOCEAN_TOKEN environment variable is not set")
 		}
+	case cliClient.ProviderGCP:
+		if !gcpInEnv() {
+			term.Warn("GCP provider was selected, but GCP_PROJECT_ID environment variable is not set")
+		}
 	case cliClient.ProviderDefang:
 		// Ignore any env vars when explicitly using the Defang playground provider
 		extraMsg = "; consider using BYOC (https://s.defang.io/byoc)"
@@ -1203,13 +1215,13 @@ func determineProviderID(ctx context.Context, loader cliClient.Loader) (string,
 		var err error
 		projectName, err = loader.LoadProjectName(ctx)
 		if err != nil {
-			term.Warn("Unable to load project:", err)
+			term.Warnf("Unable to load project: %v", err)
 		}
 
 		if projectName != "" && !RootCmd.PersistentFlags().Changed("provider") { // If user manually selected auto provider, do not load from remote
 			resp, err := client.GetSelectedProvider(ctx, &defangv1.GetSelectedProviderRequest{Project: projectName})
 			if err != nil {
-				term.Warn("Unable to get selected provider:", err)
+				term.Warnf("Unable to get selected provider: %v", err)
 			} else if resp.Provider != defangv1.Provider_PROVIDER_UNSPECIFIED {
 				providerID.SetEnumValue(resp.Provider)
 				return "stored preference", nil
@@ -1251,7 +1263,7 @@ func determineProviderID(ctx context.Context, loader cliClient.Loader) (string,
 	// Save the selected provider to the fabric
 	if projectName != "" {
 		if err := client.SetSelectedProvider(ctx, &defangv1.SetSelectedProviderRequest{Project: projectName, Provider: providerID.EnumValue()}); err != nil {
-			term.Warn("Unable to save selected provider to defang server:", err)
+			term.Warnf("Unable to save selected provider to defang server: %v", err)
 		} else {
 			term.Printf("%v is now the default provider for project %v and will auto-select next time if no other provider is specified. Use --provider=auto to reselect.", providerID, projectName)
 		}

src/cmd/cli/command/compose.go

@@ -395,7 +395,6 @@ func makeComposePsCmd() *cobra.Command {
 				}
 
 				term.Warn(err)
-
 				printDefangHint("To start a new project, do:", "new")
 				return nil
 			}

src/pkg/cli/cert.go

@@ -235,7 +235,7 @@ func waitForCNAME(ctx context.Context, domain string, targets []string, client c
 		if serverSideVerified || serverVerifyRpcFailure >= 3 {
 			locallyVerified := dns.CheckDomainDNSReady(ctx, domain, targets)
 			if serverSideVerified && !locallyVerified {
-				term.Warnf("The DNS configuration for %v has been successfully verified. However, your local environment may still be using cached data, so it could take several minutes for the DNS changes to propagate on your system.", domain)
+				term.Warnf("DNS settings for %v are verified, but changes may take a few minutes to propagate due to caching.", domain)
 				return nil
 			}
 			if locallyVerified {

src/pkg/cli/compose/config_detector.go

@@ -23,13 +23,13 @@ func detectConfig(input string) (detectorTypes []string, err error) {
 	// create a scanner from scanner config
 	scannerClient, err := scanner.NewScannerFromConfig(cfg)
 	if err != nil {
-		return nil, fmt.Errorf("Failed to make a config detector: %w", err)
+		return nil, fmt.Errorf("failed to make a config detector: %w", err)
 	}
 
 	// scan the input
 	ds, err := scannerClient.Scan(input)
 	if err != nil {
-		return nil, fmt.Errorf("Failed to scan input: %w", err)
+		return nil, fmt.Errorf("failed to scan input: %w", err)
 	}
 
 	// return a list of detector types

src/pkg/cli/compose/context.go

@@ -294,7 +294,7 @@ func createTarball(ctx context.Context, root, dockerfile string) (*bytes.Buffer,
 
 		fileCount++
 		if fileCount == ContextFileLimit+1 {
-			term.Warnf("The build context contains more than %d files; use --debug or create .dockerignore to exclude caches and build artifacts", ContextFileLimit)
+			term.Warnf("the build context contains more than %d files; use --debug or create .dockerignore to exclude caches and build artifacts", ContextFileLimit)
 		}
 
 		bufLen := buf.Len()
@@ -303,7 +303,7 @@ func createTarball(ctx context.Context, root, dockerfile string) (*bytes.Buffer,
 			return fmt.Errorf("the build context is limited to %s; consider downloading large files in the Dockerfile or set the DEFANG_BUILD_CONTEXT_LIMIT environment variable", units.BytesSize(float64(ContextSizeHardLimit)))
 		}
 		if bufLen <= ContextSizeSoftLimit && buf.Len() > ContextSizeSoftLimit {
-			term.Warnf("The build context is more than %s; use --debug or create .dockerignore to exclude caches and build artifacts", units.BytesSize(float64(buf.Len())))
+			term.Warnf("the build context is larger than %s; use --debug or create .dockerignore to exclude caches and build artifacts", units.BytesSize(float64(buf.Len())))
 		}
 		return err
 	})

src/pkg/cli/compose/convert.go

@@ -27,7 +27,7 @@ func fixupPort(port *composeTypes.ServicePortConfig) {
 				term.Debugf("port %d: ignoring 'published: %s' in 'ingress' mode", port.Target, port.Published)
 			}
 			if (port.Protocol == Protocol_TCP || port.Protocol == Protocol_UDP) && port.AppProtocol != "http" {
-				// term.Warnf("TCP ingress is not supported; assuming HTTP (remove 'protocol' to silence)")
+				// TCP ingress is not supported; assuming HTTP (remove 'protocol' to silence)"
 				port.AppProtocol = "http"
 			}
 			break

src/pkg/cli/compose/fixup.go

@@ -5,6 +5,7 @@ import (
 	"slices"
 	"strconv"
 
+	"github.com/DefangLabs/defang/src/pkg"
 	"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"
@@ -42,16 +43,21 @@ func FixupServices(ctx context.Context, provider client.Provider, project *types
 			}
 			svccfg.Build.Context = url
 
+			removedArgs := []string{}
 			for key, value := range svccfg.Build.Args {
 				if key == "" || value == nil {
-					term.Warnf("service %q: skipping unset build argument %q", svccfg.Name, key)
+					removedArgs = append(removedArgs, key)
 					delete(svccfg.Build.Args, key) // remove the empty key; this is safe
 					continue
 				}
 
 				val := svcNameReplacer.ReplaceServiceNameWithDNS(svccfg.Name, key, *value, BuildArgs)
 				svccfg.Build.Args[key] = &val
 			}
+
+			if len(removedArgs) > 0 {
+				term.Warnf("service %q: skipping unset build argument %s", svccfg.Name, pkg.QuotedArray(removedArgs))
+			}
 		}
 
 		// Fixup secret references; secrets are supposed to be files, not env, but it's kept for backward compatibility
@@ -64,10 +70,16 @@ func FixupServices(ctx context.Context, provider client.Provider, project *types
 		svccfg.Secrets = nil
 
 		// Fixup environment variables
+		shownOnce := false
+		useCfg := []string{}
+		overriddenCfg := []string{}
 		for key, value := range svccfg.Environment {
 			// A bug in Compose-go env file parsing can cause empty keys
 			if key == "" {
-				term.Warnf("service %q: skipping unset environment variable key", svccfg.Name)
+				if !shownOnce {
+					term.Warnf("service %q: skipping unset environment variable key", svccfg.Name)
+					shownOnce = true
+				}
 				delete(svccfg.Environment, key) // remove the empty key; this is safe
 				continue
 			}
@@ -78,9 +90,9 @@ func FixupServices(ctx context.Context, provider client.Provider, project *types
 			// Check if the environment variable is an existing config; if so, mark it as such
 			if _, ok := slices.BinarySearch(config.Names, key); ok {
 				if svcNameReplacer.HasServiceName(*value) {
-					term.Warnf("service %q: environment variable %q will use the `defang config` value instead of adjusted service name", svccfg.Name, key)
+					useCfg = append(useCfg, key)
 				} else {
-					term.Warnf("service %q: environment variable %q overridden by config", svccfg.Name, key)
+					overriddenCfg = append(overriddenCfg, key)
 				}
 				svccfg.Environment[key] = nil
 				continue
@@ -90,6 +102,14 @@ func FixupServices(ctx context.Context, provider client.Provider, project *types
 			svccfg.Environment[key] = &val
 		}
 
+		if len(useCfg) > 0 {
+			term.Warnf("service %q: environment variable(s) %s will use the `defang config` value instead of adjusted service name", svccfg.Name, pkg.QuotedArray(useCfg))
+		}
+
+		if len(overriddenCfg) > 0 {
+			term.Warnf("service %q: environment variable(s) %s overridden by config", svccfg.Name, pkg.QuotedArray(overriddenCfg))
+		}
+
 		_, redis := svccfg.Extensions["x-defang-redis"]
 		if redis {
 			if _, ok := provider.(*client.PlaygroundProvider); ok {

src/pkg/cli/compose/loader.go

@@ -155,7 +155,7 @@ func (c *Loader) NewProjectOptions() (*cli.ProjectOptions, error) {
 						if inEnv {
 							term.Warnf("Environment variable %q is not used; add it to `.env` if needed", key)
 						} else {
-							term.Debugf("Unresolved variable %s", key)
+							term.Debugf("Unresolved environment variable %q", key)
 						}
 						return "", false
 					}

src/pkg/cli/compose/validate.go

@@ -42,6 +42,7 @@ func ValidateService(service *types.ServiceConfig) error {
 		// hasHost = hasHost || port.Mode == v1.Mode_HOST
 		uniquePorts[port.Target] = true
 	}
+
 	if service.HealthCheck != nil && len(service.HealthCheck.Test) > 0 {
 		// Technically this should test for <= but both interval and timeout have 30s as the default value in compose spec
 		interval := getOrZero(service.HealthCheck.Interval)

src/pkg/cli/composeUp.go

@@ -130,7 +130,7 @@ func ComposeUp(ctx context.Context, project *compose.Project, c client.FabricCli
 			},
 		})
 		if err != nil {
-			term.Debug("PutDeployment failed:", err)
+			term.Debugf("PutDeployment failed: %v", err)
 			term.Warn("Unable to update deployment history, but deployment will proceed anyway.")
 		}
 	}

src/pkg/cli/configList.go

@@ -22,7 +22,7 @@ func ConfigList(ctx context.Context, projectName string, provider client.Provide
 
 	numConfigs := len(config.Names)
 	if numConfigs == 0 {
-		_, err := term.Warnf("No configs found")
+		_, err := term.Warn("No configs found")
 		return err
 	}
 

src/pkg/cli/generate.go

@@ -22,7 +22,7 @@ type GenerateArgs struct {
 
 func GenerateWithAI(ctx context.Context, client client.FabricClient, args GenerateArgs) ([]string, error) {
 	if DoDryRun {
-		term.Warn("Dry run, not generating files")
+		term.Warn("Dry run, no project files will be generated")
 		return nil, ErrDryRun
 	}
 

src/pkg/cli/login.go

@@ -78,7 +78,7 @@ func InteractiveLogin(ctx context.Context, client client.FabricClient, gitHubCli
 	term.Info("Successfully logged in to", host, "("+tenant.String()+" tenant)")
 
 	if err := saveAccessToken(fabric, at); err != nil {
-		term.Warn("Failed to save access token:", err)
+		term.Warnf("Failed to save access token, try re-authenticating: %v", err)
 	}
 	return nil
 }

src/pkg/cli/new.go

@@ -114,7 +114,7 @@ func copyFromSamples(ctx context.Context, dir string, names []string, skipExisti
 					if !skipExisting || !os.IsExist(err) {
 						return err
 					}
-					term.Warnf("File %q already exists, skipping", path)
+					term.Warnf("File already exists, skipping: %q", path)
 				}
 			}
 		}

src/pkg/cli/tail.go

@@ -188,7 +188,7 @@ func Tail(ctx context.Context, provider client.Provider, projectName string, opt
 			if _, err := provider.GetService(ctx, &defangv1.GetRequest{Project: projectName, Name: service}); err != nil {
 				switch connect.CodeOf(err) {
 				case connect.CodeNotFound:
-					term.Warn("Service does not exist (yet):", service)
+					term.Warnf("Service does not exist (yet): %q", service)
 				case connect.CodeUnknown:
 					// Ignore unknown (nil) errors
 				default:

src/pkg/clouds/do/appPlatform/setup.go

@@ -229,7 +229,7 @@ func waitForActiveDeployment(ctx context.Context, apps godo.AppsService, appID s
 		attempts++
 		pkg.SleepWithContext(ctx, 10*time.Second) // was changed from time.Sleep
 	}
-	return fmt.Errorf("timeout waiting to app (%s) deployment", appID)
+	return fmt.Errorf("timeout waiting for app (%s) deployment", appID)
 }
 
 func NewClient(ctx context.Context) *godo.Client {

src/pkg/clouds/gcp/artifactregistry.go

@@ -14,14 +14,14 @@ import (
 func (gcp Gcp) EnsureArtifactRegistryExists(ctx context.Context, repoName string) (string, error) {
 	client, err := artifactregistry.NewClient(ctx)
 	if err != nil {
-		return "", fmt.Errorf("artifactregistry.NewClient: %w", err)
+		return "", fmt.Errorf("failed to create artifactregistry client: %w", err)
 	}
 
 	parent := fmt.Sprintf("projects/%s/locations/%s", gcp.ProjectId, gcp.Region)
 	fullRepoName := fmt.Sprintf("%s/repositories/%s", parent, repoName)
 	if resp, err := client.GetRepository(ctx, &artifactregistrypb.GetRepositoryRequest{Name: fullRepoName}); err != nil {
 		if IsNotFound(err) {
-			return "", fmt.Errorf("artifactregistry.GetRepository: %w", err)
+			return "", fmt.Errorf("failed to get artifactregistry repository: %w", err)
 		}
 	} else if resp != nil {
 		return resp.Name, nil

src/pkg/clouds/gcp/cloudrun.go

@@ -157,7 +157,7 @@ func (gcp Gcp) GetExecutionEnv(ctx context.Context, executionName string) (map[s
 
 	executionName = path.Base(executionName)
 	if len(executionName) < 6 {
-		return nil, fmt.Errorf("invalid execution name %q", executionName)
+		return nil, fmt.Errorf("invalid execution name, must be longer than 6 characters: %q", executionName)
 	}
 	jobName := executionName[:len(executionName)-6]
 

src/pkg/clouds/gcp/dns.go

@@ -10,7 +10,7 @@ import (
 func (gcp Gcp) EnsureDNSZoneExists(ctx context.Context, name, domain, description string) (*dns.ManagedZone, error) {
 	dnsSvc, err := dns.NewService(ctx)
 	if err != nil {
-		return nil, fmt.Errorf("dns.NewService: %w", err)
+		return nil, fmt.Errorf("unable to check DNS zone, failed to create DNS service: %w", err)
 	}
 
 	zoneSvc := dns.NewManagedZonesService(dnsSvc)
@@ -20,7 +20,7 @@ func (gcp Gcp) EnsureDNSZoneExists(ctx context.Context, name, domain, descriptio
 	}
 
 	if !IsNotFound(err) {
-		return nil, fmt.Errorf("zoneSvc.Get: %w", err)
+		return nil, fmt.Errorf("failed to get DNS managed zone service: %w", err)
 	}
 
 	if domain[len(domain)-1] != '.' {
@@ -31,7 +31,7 @@ func (gcp Gcp) EnsureDNSZoneExists(ctx context.Context, name, domain, descriptio
 		DnsName:     domain,
 		Description: description,
 	}).Do(); err != nil {
-		return nil, fmt.Errorf("zoneSvc.Create: %w", err)
+		return nil, fmt.Errorf("failed to create zone service: %w", err)
 	} else {
 		return zone, nil
 	}
@@ -40,13 +40,13 @@ func (gcp Gcp) EnsureDNSZoneExists(ctx context.Context, name, domain, descriptio
 func (gcp Gcp) GetDNSZone(ctx context.Context, name string) (*dns.ManagedZone, error) {
 	dnsSvc, err := dns.NewService(ctx)
 	if err != nil {
-		return nil, fmt.Errorf("dns.NewService: %w", err)
+		return nil, fmt.Errorf("unable to get DNS zone, failed to create DNS service: %w", err)
 	}
 
 	zoneSvc := dns.NewManagedZonesService(dnsSvc)
 	zone, err := zoneSvc.Get(gcp.ProjectId, name).Do()
 	if err != nil {
-		return nil, fmt.Errorf("zoneSvc.Get: %w", err)
+		return nil, fmt.Errorf("failed to get DNS zone service: %w", err)
 	}
 
 	return zone, nil

src/pkg/clouds/gcp/iam.go

@@ -23,7 +23,7 @@ import (
 func (gcp Gcp) EnsureRoleExists(ctx context.Context, roleId, title, description string, permissions []string) (string, error) {
 	client, err := iamadm.NewIamClient(ctx)
 	if err != nil {
-		return "", fmt.Errorf("iam.NewIamClient: %w", err)
+		return "", fmt.Errorf("unable to ensure role exists, failed to create Iam client: %w", err)
 	}
 	defer client.Close()
 
@@ -91,7 +91,7 @@ func (gcp Gcp) EnsureRoleExists(ctx context.Context, roleId, title, description
 func (gcp Gcp) EnsureServiceAccountExists(ctx context.Context, serviceAccountId, displayName, description string) (string, error) {
 	client, err := iamadm.NewIamClient(ctx)
 	if err != nil {
-		return "", fmt.Errorf("iam.NewIamClient: %w", err)
+		return "", fmt.Errorf("unable to ensure Service Account exists, failed to create Iam Client: %w", err)
 	}
 	defer client.Close()
 
@@ -218,7 +218,7 @@ func (gcp Gcp) EnsureServiceAccountHasBucketRoles(ctx context.Context, bucketNam
 func (gcp Gcp) EnsureServiceAccountHasArtifactRegistryRoles(ctx context.Context, repo, serviceAccount string, roles []string) error {
 	client, err := artifactregistry.NewClient(ctx)
 	if err != nil {
-		return fmt.Errorf("failed to create artifact registry client: %w", err)
+		return fmt.Errorf("unable to ensure service account artifact registry role, failed to create artifact registry client: %w", err)
 	}
 	defer client.Close()
 
@@ -229,7 +229,7 @@ func (gcp Gcp) EnsureServiceAccountHasArtifactRegistryRoles(ctx context.Context,
 func (gcp Gcp) EnsureUserHasServiceAccountRoles(ctx context.Context, user, serviceAccount string, roles []string) error {
 	client, err := iamadm.NewIamClient(ctx)
 	if err != nil {
-		log.Fatalf("failed to create artifact registry client: %v", err)
+		log.Fatalf("unable to ensure user service account role, failed to create artifact registry client: %v", err)
 	}
 	defer client.Close()