Normalize project name by replacing underscore with dash #1724
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,7 +10,7 @@ logName="projects/test-project/logs/docker-logs" | |
| labels."defang-etag" = "test-etag" | ||
| ) OR ( | ||
| resource.type="build" | ||
| labels.build_tags =~ "beta__[a-zA-Z0-9-_]{1,63}_test-etag" | ||
| -labels.build_step="MAIN" | ||
| ) OR ( | ||
| resource.type="cloud_run_revision" | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,7 +10,7 @@ logName="projects/test-project/logs/docker-logs" | |
| labels."defang-etag" = "test-etag" | ||
| ) OR ( | ||
| resource.type="build" | ||
| labels.build_tags =~ "beta__[a-zA-Z0-9-_]{1,63}_test-etag" | ||
| -labels.build_step="MAIN" | ||
| ) OR ( | ||
| resource.type="cloud_run_revision" | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,7 +12,7 @@ logName="projects/test-project/logs/docker-logs" | |
| labels."defang-stack" = "beta" | ||
| ) OR ( | ||
| resource.type="build" | ||
| labels.build_tags =~ "beta__[a-zA-Z0-9-_]{1,63}_" | ||
| -labels.build_step="MAIN" | ||
| ) OR ( | ||
| resource.type="cloud_run_revision" | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,7 +14,7 @@ logName="projects/test-project/logs/docker-logs" | |
| labels."defang-etag" = "test-etag" | ||
| ) OR ( | ||
| resource.type="build" | ||
| labels.build_tags =~ "beta_test-project_[a-zA-Z0-9-_]{1,63}_test-etag" | ||
| -labels.build_step="MAIN" | ||
| ) OR ( | ||
| resource.type="cloud_run_revision" | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,7 +9,7 @@ logName="projects/test-project/logs/docker-logs" | |
| labels."defang-stack" = "beta" | ||
| ) OR ( | ||
| resource.type="build" | ||
| labels.build_tags =~ "beta__[a-zA-Z0-9-_]{1,63}_" | ||
| -labels.build_step="MAIN" | ||
| ) OR ( | ||
| resource.type="cloud_run_revision" | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,7 +9,7 @@ logName="projects/test-project/logs/docker-logs" | |
| labels."defang-stack" = "beta" | ||
| ) OR ( | ||
| resource.type="build" | ||
| labels.build_tags =~ "beta__[a-zA-Z0-9-_]{1,63}_" | ||
|
Contributor
There was a problem hiding this comment. 🧩 Analysis chain🏁 Script executed: # Find the test file and check its content
find . -name "with_logtype_build.query" -type f | head -5Repository: DefangLabs/defang Length of output: 124 🏁 Script executed: # Search for query.go in the codebase
find . -name "query.go" -path "*/byoc/gcp/*" -type fRepository: DefangLabs/defang Length of output: 99 🏁 Script executed: # Look for the pattern in production code that generates these queries
rg -l "build_tags" --type go --type txt | head -10Repository: DefangLabs/defang Length of output: 135 🏁 Script executed: # Read the test file to see the context and line 12
cat -n ./src/pkg/cli/client/byoc/gcp/testdata/with_logtype_build.query | head -20Repository: DefangLabs/defang Length of output: 571 🏁 Script executed: # Read query.go to find the regex pattern construction
cat -n ./src/pkg/cli/client/byoc/gcp/query.go | head -150Repository: DefangLabs/defang Length of output: 4370 🏁 Script executed: # Continue reading query.go to find the AddBuildLogQuery or similar method
sed -n '150,250p' ./src/pkg/cli/client/byoc/gcp/query.go | cat -nRepository: DefangLabs/defang Length of output: 4133 🏁 Script executed: # Search for where the build_tags pattern is defined
rg -n "build_tags" ./src/pkg/cli/client/byoc/gcp/query.go -A 3 -B 3Repository: DefangLabs/defang Length of output: 427 🏁 Script executed: # Verify the regex issue by checking what characters are actually matched
# Create a test to show the unintended range
python3 << 'EOF'
import re
# The problematic pattern
pattern = r'[a-zA-Z0-9-_]'
regex = re.compile(pattern)
# Test various characters
test_chars = list(range(48, 96)) + [95] # ASCII 48-95
matched = []
for code in test_chars:
char = chr(code)
if regex.match(char):
matched.append((code, char, repr(char)))
print("Characters matched by [a-zA-Z0-9-_]:")
for code, char, repr_char in matched:
if char in '0123456789':
print(f" {code}: {repr_char} (digit - INTENDED)")
elif char in 'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ':
print(f" {code}: {repr_char} (letter - INTENDED)")
elif char in '-_':
print(f" {code}: {repr_char} (dash/underscore - INTENDED)")
else:
print(f" {code}: {repr_char} (UNINTENDED - in range 57-95)")
print("\n\nCharacters matched by corrected [a-zA-Z0-9_-]:")
pattern_fixed = r'[a-zA-Z0-9_-]'
regex_fixed = re.compile(pattern_fixed)
for code in test_chars:
char = chr(code)
if regex_fixed.match(char):
matched_fixed = (code, char, repr(char))
if char not in [c[1] for c in matched]:
print(f" {code}: {repr(char)} (NOW EXCLUDED - UNINTENDED)")
EOFRepository: DefangLabs/defang Length of output: 147 Critical: Fix hyphen placement in character class to prevent unintended character matches. The character class Move the hyphen to the end: This pattern appears in both the test data (line 12) and the production code ( 🤖 Prompt for AI Agents |
||
| -labels.build_step="MAIN" | ||
| ) | ||
| ) | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,7 +10,7 @@ logName="projects/test-project/logs/docker-logs" | |
| labels."defang-project" = "test-project" | ||
| ) OR ( | ||
| resource.type="build" | ||
| labels.build_tags =~ "beta_test-project_[a-zA-Z0-9-_]{1,63}_" | ||
| -labels.build_step="MAIN" | ||
| ) OR ( | ||
| resource.type="cloud_run_revision" | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,7 +10,7 @@ logName="projects/test-project/logs/docker-logs" | |
| labels."defang-project" = "test-project" | ||
| ) OR ( | ||
| resource.type="build" | ||
| labels.build_tags =~ "beta_test-project_[a-zA-Z0-9-_]{1,63}_" | ||
| -labels.build_step="MAIN" | ||
| ) OR ( | ||
| resource.type="cloud_run_revision" | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -5,6 +5,7 @@ import ( | |||||||||||||||||||||||||||||||
| "fmt" | ||||||||||||||||||||||||||||||||
| "os" | ||||||||||||||||||||||||||||||||
| "path/filepath" | ||||||||||||||||||||||||||||||||
| "regexp" | ||||||||||||||||||||||||||||||||
| "slices" | ||||||||||||||||||||||||||||||||
| "strconv" | ||||||||||||||||||||||||||||||||
| "strings" | ||||||||||||||||||||||||||||||||
|
|
@@ -29,6 +30,12 @@ func FixupServices(ctx context.Context, provider client.Provider, project *compo | |||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| oldName := project.Name | ||||||||||||||||||||||||||||||||
| project.Name = NormalizeProjectName(project.Name) | ||||||||||||||||||||||||||||||||
| if project.Name != oldName { | ||||||||||||||||||||||||||||||||
| term.Debugf("normalized project name %q -> %q", oldName, project.Name) | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| // Preload the current config so we can detect which environment variables should be passed as "secrets" | ||||||||||||||||||||||||||||||||
| config, err := provider.ListConfig(ctx, &defangv1.ListConfigsRequest{Project: project.Name}) | ||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||
|
|
@@ -477,3 +484,9 @@ func IsRedisRepo(repo string) bool { | |||||||||||||||||||||||||||||||
| func IsMongoRepo(repo string) bool { | ||||||||||||||||||||||||||||||||
| return strings.HasSuffix(repo, "mongo") | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| var safeProjectNameRE = regexp.MustCompile(`[^A-Za-z0-9-]+`) | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| func NormalizeProjectName(name string) string { | ||||||||||||||||||||||||||||||||
| return safeProjectNameRE.ReplaceAllString(name, "-") | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
Comment on lines
+488
to
+492
Contributor
There was a problem hiding this comment. Handle edge cases in NormalizeProjectName. The current implementation can produce invalid project names in edge cases:
These could cause downstream failures in GCP APIs. Consider adding safeguards similar to
🔎 Proposed enhancement with edge case handling+var safeProjectNameRE = regexp.MustCompile(`[^A-Za-z0-9-]+`)
+
func NormalizeProjectName(name string) string {
- return safeProjectNameRE.ReplaceAllString(name, "-")
+ normalized := safeProjectNameRE.ReplaceAllString(name, "-")
+ normalized = strings.Trim(normalized, "-")
+ if normalized == "" {
+ return "project"
+ }
+ return normalized
}Import 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
Member
There was a problem hiding this comment. This seems like a good suggestion. Let's write some test cases for this function and cover the suggested cases. I vaguely remember that repeated hyphens may also not be permitted. Is that true?
Contributor
There was a problem hiding this comment. 🧩 Analysis chain🌐 Web query: 💡 Result: Short answer — two different fields:
Notes:
References:
Citations:
🏁 Script executed: Length of output: 4114 @jordanstephens, great question! Based on GCP Project ID constraints (6-30 characters, lowercase letters/numbers/hyphens only, must start with a letter, cannot end with a hyphen), repeated hyphens aren't explicitly forbidden, but they're poor practice and should be collapsed. More importantly, the current implementation has gaps:
Here's a comprehensive test file covering all edge cases: package compose
import (
"testing"
)
func TestNormalizeProjectName(t *testing.T) {
tests := []struct {
name string
input string
expected string
}{
{
name: "simple alphanumeric",
input: "myproject",
expected: "myproject",
},
{
name: "with valid hyphens",
input: "my-project-123",
expected: "my-project-123",
},
{
name: "underscore to hyphen",
input: "my_project",
expected: "my-project",
},
{
name: "multiple underscores",
input: "my__project",
expected: "my-project",
},
{
name: "leading underscore",
input: "_test",
expected: "test",
},
{
name: "trailing underscore",
input: "test_",
expected: "test",
},
{
name: "leading and trailing underscores",
input: "_test_",
expected: "test",
},
{
name: "only underscores",
input: "___",
expected: "project",
},
{
name: "empty string",
input: "",
expected: "project",
},
{
name: "special characters",
input: "my@project#test",
expected: "my-project-test",
},
{
name: "mixed special characters and underscores",
input: "my_project@123",
expected: "my-project-123",
},
{
name: "spaces",
input: "my project",
expected: "my-project",
},
{
name: "dots",
input: "my.project.test",
expected: "my-project-test",
},
{
name: "consecutive special characters",
input: "my___project!!!",
expected: "my-project",
},
{
name: "starts with number",
input: "123project",
expected: "123project",
},
{
name: "starts with hyphen after normalization",
input: "-test",
expected: "test",
},
{
name: "ends with hyphen after normalization",
input: "test-",
expected: "test",
},
{
name: "only special characters",
input: "!!!@@@",
expected: "project",
},
{
name: "unicode characters",
input: "my项目test",
expected: "my-test",
},
{
name: "real world example from issue",
input: "raphaeltm-prod1_zaoconnect_project",
expected: "raphaeltm-prod1-zaoconnect-project",
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
result := NormalizeProjectName(tt.input)
if result != tt.expected {
t.Errorf("NormalizeProjectName(%q) = %q, want %q", tt.input, result, tt.expected)
}
// Additional validation: result should not have leading/trailing hyphens
if len(result) > 0 {
if result[0] == '-' {
t.Errorf("NormalizeProjectName(%q) = %q starts with hyphen", tt.input, result)
}
if result[len(result)-1] == '-' {
t.Errorf("NormalizeProjectName(%q) = %q ends with hyphen", tt.input, result)
}
}
// Result should not be empty
if result == "" {
t.Errorf("NormalizeProjectName(%q) returned empty string", tt.input)
}
})
}
}
func TestNormalizeProjectNameRepeatedHyphens(t *testing.T) {
tests := []struct {
name string
input string
expected string
}{
{
name: "double hyphen",
input: "my--project",
expected: "my-project",
},
{
name: "triple hyphen",
input: "my---project",
expected: "my-project",
},
{
name: "multiple double hyphens",
input: "my--project--test",
expected: "my-project-test",
},
{
name: "from consecutive special chars",
input: "my___project",
expected: "my-project",
},
{
name: "mixed underscores and special chars",
input: "my_@_project",
expected: "my-project",
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
result := NormalizeProjectName(tt.input)
if result != tt.expected {
t.Errorf("NormalizeProjectName(%q) = %q, want %q", tt.input, result, tt.expected)
}
// Verify no consecutive hyphens remain
if len(result) > 1 {
for i := 0; i < len(result)-1; i++ {
if result[i] == '-' && result[i+1] == '-' {
t.Errorf("NormalizeProjectName(%q) = %q contains consecutive hyphens", tt.input, result)
}
}
}
})
}
}And here's the enhanced implementation to handle all these cases: var safeProjectNameRE = regexp.MustCompile(`[^A-Za-z0-9-]+`)
var repeatedHyphensRE = regexp.MustCompile(`-+`)
func NormalizeProjectName(name string) string {
// Replace non-allowed characters with hyphens
normalized := safeProjectNameRE.ReplaceAllString(name, "-")
// Collapse consecutive hyphens into single hyphen
normalized = repeatedHyphensRE.ReplaceAllString(normalized, "-")
// Trim leading and trailing hyphens
normalized = strings.Trim(normalized, "-")
// Return default if empty
if normalized == "" {
return "project"
}
return normalized
}The key improvements:
Note: I didn't enforce "must start with letter" since that might be too restrictive for project names (vs. GCP Project IDs), but you can add that if needed. 🧬 Code Graph Analysis Results[src/pkg/cli/compose/fixup.go] — Summary of relevant code for user comment
🧠 Learnings used |
||||||||||||||||||||||||||||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -77,7 +77,7 @@ func ComposeUp(ctx context.Context, fabric client.FabricClient, provider client. | |
| } | ||
|
|
||
| delegateDomain, err := fabric.GetDelegateSubdomainZone(ctx, &defangv1.GetDelegateSubdomainZoneRequest{ | ||
| Project: fixedProject.Name, | ||
| Stack: provider.GetStackNameForDomain(), | ||
| }) | ||
| if err != nil { | ||
|
|
@@ -87,15 +87,15 @@ func ComposeUp(ctx context.Context, fabric client.FabricClient, provider client. | |
|
|
||
| deployRequest := &defangv1.DeployRequest{ | ||
| Mode: mode.Value(), | ||
| Project: fixedProject.Name, | ||
| Compose: bytes, | ||
| DelegateDomain: delegateDomain.Zone, | ||
| } | ||
|
|
||
| delegation, err := provider.PrepareDomainDelegation(ctx, client.PrepareDomainDelegationRequest{ | ||
| DelegateDomain: delegateDomain.Zone, | ||
| Preview: upload == compose.UploadModePreview || upload == compose.UploadModeEstimate, | ||
| Project: fixedProject.Name, | ||
| }) | ||
| if err != nil { | ||
| return nil, project, err | ||
|
|
@@ -115,7 +115,7 @@ func ComposeUp(ctx context.Context, fabric client.FabricClient, provider client. | |
| if delegation != nil && len(delegation.NameServers) > 0 { | ||
| req := &defangv1.DelegateSubdomainZoneRequest{ | ||
| NameServerRecords: delegation.NameServers, | ||
| Project: fixedProject.Name, | ||
| Stack: provider.GetStackNameForDomain(), | ||
| } | ||
| _, err = fabric.CreateDelegateSubdomainZone(ctx, req) | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ import ( | |
| "errors" | ||
|
|
||
| "github.com/DefangLabs/defang/src/pkg/cli/client" | ||
| "github.com/DefangLabs/defang/src/pkg/cli/compose" | ||
| "github.com/DefangLabs/defang/src/pkg/term" | ||
| "github.com/DefangLabs/defang/src/pkg/types" | ||
| defangv1 "github.com/DefangLabs/defang/src/protos/io/defang/v1" | ||
|
|
@@ -29,7 +30,7 @@ func WaitServiceState( | |
| } | ||
|
|
||
| // Assume "services" are normalized service names | ||
| subscribeRequest := defangv1.SubscribeRequest{Project: compose.NormalizeProjectName(projectName), Etag: etag, Services: services} | ||
|
Member
There was a problem hiding this comment. Should we push this responsibility into the |
||
| serverStream, err := provider.Subscribe(ctx, &subscribeRequest) | ||
| if err != nil { | ||
| return nil, err | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,7 @@ import ( | |
|
|
||
| "github.com/DefangLabs/defang/src/pkg" | ||
| "github.com/DefangLabs/defang/src/pkg/cli/client" | ||
| "github.com/DefangLabs/defang/src/pkg/cli/compose" | ||
| "github.com/DefangLabs/defang/src/pkg/dryrun" | ||
| "github.com/DefangLabs/defang/src/pkg/logs" | ||
| "github.com/DefangLabs/defang/src/pkg/spinner" | ||
|
|
@@ -207,7 +208,7 @@ func streamLogs(ctx context.Context, provider client.Provider, projectName strin | |
| Etag: options.Deployment, | ||
| LogType: uint32(options.LogType), | ||
| Pattern: options.Filter, | ||
| Project: compose.NormalizeProjectName(projectName), // Matching the FixupServices behavior | ||
|
Comment on lines
-210
to
+211
Member
There was a problem hiding this comment. Same here, I think |
||
| Services: options.Services, | ||
| Since: sinceTs, // this is also used to continue from the last timestamp | ||
| Until: untilTs, | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -54,7 +54,7 @@ type BuildTag struct { | |
|
|
||
| func (bt BuildTag) String() string { | ||
| if bt.Stack == "" { | ||
| return fmt.Sprintf("%s_%s_%s", bt.Project, bt.Service, bt.Etag) // Backward compatibility | ||
| } else { | ||
| return fmt.Sprintf("%s_%s_%s_%s", bt.Stack, bt.Project, bt.Service, bt.Etag) | ||
| } | ||
|
|
@@ -67,20 +67,20 @@ func (bt *BuildTag) Parse(tags []string) error { | |
| continue | ||
| } | ||
| parts := strings.Split(tag, "_") | ||
| if len(parts) < 3 { | ||
| return fmt.Errorf("invalid cloudbuild build tags value: %q", tag) | ||
| } | ||
|
|
||
| if n := len(parts); n == 3 { // Backward compatibility | ||
| bt.Stack = "" | ||
| bt.Project = parts[0] | ||
| bt.Service = parts[1] | ||
| bt.Etag = parts[2] | ||
| } else { | ||
| bt.Stack = parts[0] | ||
| bt.Project = parts[1] // Project names has been normalized to not contain underscores | ||
| bt.Service = strings.Join(parts[2:n-1], "_") // Service names may contain underscores, so join all parts except last which is the etag | ||
| bt.Etag = parts[n-1] | ||
| } | ||
| } | ||
| return nil | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,47 @@ | ||||||||||||||
| package gcp | ||||||||||||||
|
|
||||||||||||||
| import ( | ||||||||||||||
| "testing" | ||||||||||||||
| ) | ||||||||||||||
|
|
||||||||||||||
| func TestBuildTagString(t *testing.T) { | ||||||||||||||
| tests := []struct { | ||||||||||||||
| name string | ||||||||||||||
| bt BuildTag | ||||||||||||||
| want string | ||||||||||||||
| }{ | ||||||||||||||
| { | ||||||||||||||
| name: "with stack", | ||||||||||||||
| bt: BuildTag{Stack: "stack1", Project: "proj", Service: "svc", Etag: "123"}, | ||||||||||||||
| want: "stack1_proj_svc_123", | ||||||||||||||
| }, | ||||||||||||||
| { | ||||||||||||||
| name: "without stack", | ||||||||||||||
| bt: BuildTag{Project: "proj", Service: "svc", Etag: "123"}, | ||||||||||||||
| want: "proj_svc_123", | ||||||||||||||
| }, | ||||||||||||||
| { | ||||||||||||||
| name: "service name with underscores", | ||||||||||||||
| bt: BuildTag{Stack: "stack1", Project: "my-proj-name", Service: "svc_name", Etag: "123"}, | ||||||||||||||
| want: "stack1_my-proj-name_svc_name_123", | ||||||||||||||
| }, | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| for _, tt := range tests { | ||||||||||||||
| t.Run(tt.name, func(t *testing.T) { | ||||||||||||||
| tagStr := tt.bt.String() | ||||||||||||||
| if tagStr != tt.want { | ||||||||||||||
| t.Errorf("String() = %q, want %q", tagStr, tt.want) | ||||||||||||||
| } | ||||||||||||||
| var parsed BuildTag | ||||||||||||||
| err := parsed.Parse([]string{tagStr}) | ||||||||||||||
| if err != nil { | ||||||||||||||
| t.Fatalf("Parse() returned error: %v", err) | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| if parsed != tt.bt { | ||||||||||||||
| t.Errorf("Parse() = %+v, want %+v", parsed, tt.want) | ||||||||||||||
| } | ||||||||||||||
|
Comment on lines
+42
to
+44
Contributor
There was a problem hiding this comment. Bug: Wrong variable in error message. The error message references 🔎 Proposed fix if parsed != tt.bt {
- t.Errorf("Parse() = %+v, want %+v", parsed, tt.want)
+ t.Errorf("Parse() = %+v, want %+v", parsed, tt.bt)
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||
| }) | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2,7 +2,12 @@ package gcp | |||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| import ( | ||||||||||||||||||||||||||
| "context" | ||||||||||||||||||||||||||
| "crypto/sha256" | ||||||||||||||||||||||||||
| "encoding/binary" | ||||||||||||||||||||||||||
| "fmt" | ||||||||||||||||||||||||||
| "regexp" | ||||||||||||||||||||||||||
| "strconv" | ||||||||||||||||||||||||||
| "strings" | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| "google.golang.org/api/dns/v1" | ||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||
|
|
@@ -87,3 +92,47 @@ func (gcp Gcp) DeleteDNSZone(ctx context.Context, name string) error { | |||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| return nil | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // 1. Name must be lowercase letters, numbers, and hyphens | ||||||||||||||||||||||||||
| // 2. Name may be at most 63 characters | ||||||||||||||||||||||||||
| // 3. Name must start with a lowercase letter | ||||||||||||||||||||||||||
| // 4. Name must end with a lowercase letter or a number | ||||||||||||||||||||||||||
| var safeZoneRE = regexp.MustCompile(`[^a-z0-9-]+`) | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // Zone names have the same requirements as label values. | ||||||||||||||||||||||||||
| func SafeZoneName(input string) string { | ||||||||||||||||||||||||||
| input = strings.ToLower(input) // Rule 1: lowercase | ||||||||||||||||||||||||||
| safe := safeZoneRE.ReplaceAllString(input, "-") // Rule 1: only letters, numbers, and hyphen | ||||||||||||||||||||||||||
| safe = strings.Trim(safe, "-") // Rule 3, 4: trim hyphens from start and end | ||||||||||||||||||||||||||
| if len(safe) == 0 || safe[0] == '-' { | ||||||||||||||||||||||||||
| safe = "zone" + safe | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
Comment on lines
+107
to
+109
Member
There was a problem hiding this comment. If |
||||||||||||||||||||||||||
| if safe[0] < 'a' || safe[0] > 'z' { // Rule 3: must start with a letter | ||||||||||||||||||||||||||
| safe = "zone-" + safe | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| return hashTrim(safe, 63) // Rule 2: max length 63 | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| func hashTrim(name string, maxLength int) string { | ||||||||||||||||||||||||||
| if len(name) <= maxLength { | ||||||||||||||||||||||||||
| return name | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| const hashLength = 6 | ||||||||||||||||||||||||||
| prefix := name[:maxLength-hashLength] | ||||||||||||||||||||||||||
| suffix := name[maxLength-hashLength:] | ||||||||||||||||||||||||||
| return prefix + hashn(suffix, hashLength) | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| func hashn(str string, length int) string { | ||||||||||||||||||||||||||
| hash := sha256.New() | ||||||||||||||||||||||||||
| hash.Write([]byte(str)) | ||||||||||||||||||||||||||
| hashInt := binary.LittleEndian.Uint64(hash.Sum(nil)[:8]) | ||||||||||||||||||||||||||
| hashBase36 := strconv.FormatUint(hashInt, 36) // base 36 string | ||||||||||||||||||||||||||
| // truncate if the hash is too long | ||||||||||||||||||||||||||
| if len(hashBase36) > length { | ||||||||||||||||||||||||||
| return hashBase36[:length] | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| // if the hash is too short, pad with leading zeros | ||||||||||||||||||||||||||
| return fmt.Sprintf("%0*s", length, hashBase36) | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
Comment on lines
+133
to
+138
Contributor
There was a problem hiding this comment. Bug: In Go's Proposed fix func hashn(str string, length int) string {
hash := sha256.New()
hash.Write([]byte(str))
hashInt := binary.LittleEndian.Uint64(hash.Sum(nil)[:8])
hashBase36 := strconv.FormatUint(hashInt, 36) // base 36 string
// truncate if the hash is too long
if len(hashBase36) > length {
return hashBase36[:length]
}
// if the hash is too short, pad with leading zeros
- return fmt.Sprintf("%0*s", length, hashBase36)
+ return strings.Repeat("0", length-len(hashBase36)) + hashBase36
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
Member
There was a problem hiding this comment. I don't see a test case that covers this. Let's add one
Contributor
There was a problem hiding this comment. 🧩 Analysis chain🏁 Script executed: Length of output: 2120 @jordanstephens, great catch! Here's a test case that covers the zero-padding scenario when the hash is shorter than 6 characters: {
name: "hash trimming produces valid output without spaces",
input: strings.Repeat("a", 70), // Long enough to trigger hashTrim
validate: func(t *testing.T, output string) {
if len(output) != 63 {
t.Fatalf("expected length 63, got %d", len(output))
}
// Verify no spaces in output (would indicate improper padding)
if strings.Contains(output, " ") {
t.Fatalf("output contains spaces from improper padding: %q", output)
}
// Verify only valid characters (a-z, 0-9, -)
matched, _ := regexp.MatchString(`^[a-z0-9-]+$`, output)
if !matched {
t.Fatalf("output contains invalid characters: %q", output)
}
},
},This test should be added to the
With the current buggy code, if the hash happens to be short, this test would fail due to space padding. After applying the fix suggested by CodeRabbit, the test should pass consistently. 🧬 Code Graph Analysis Results[src/pkg/clouds/gcp/project.go] (lines 98-101) [src/pkg/clouds/gcp/project.go] (lines 15-15) [src/pkg/clouds/gcp/artifactregistry.go] (lines 51-61) |
||||||||||||||||||||||||||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this TODO done now?