toolshed/abra
toolshed/abra
4
2
Fork 18
Code Issues 29 Pull Requests 3 Packages Releases 40 Activity

fix: add warning for long secret names #359

Merged
decentral1se merged 14 commits from rix/abra:add-secret-length-linting into main 2024-04-06 21:41:37 +00:00
Conversation 11 Commits 14 Files Changed 6 +23
1 changed files with 23 additions and 0 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 81d9e03800 - Show all commits

23
cli/app/new.go
View File

@ -2,6 +2,7 @@ package app
import ( import (
"fmt" "fmt"
"strings"
"coopcloud.tech/abra/cli/internal" "coopcloud.tech/abra/cli/internal"
"coopcloud.tech/abra/pkg/autocomplete" "coopcloud.tech/abra/pkg/autocomplete"
@ -125,6 +126,10 @@ var appNewCommand = cli.Command{
logrus.Fatal(err) logrus.Fatal(err)
} }
if err := ensureSecretLengths(secrets, internal.Domain); err != nil {
logrus.Fatal(err)
}
secretCols := []string{"Name", "Value"} secretCols := []string{"Name", "Value"}
secretTable = formatter.CreateTable(secretCols) secretTable = formatter.CreateTable(secretCols)
for name, val := range secrets { for name, val := range secrets {
@ -256,3 +261,21 @@ func ensureServerFlag() error {
return nil return nil
} }
func ensureSecretLengths(secrets AppSecrets, domain string) error {
domainLength := len(domain)
decentral1se marked this conversation as resolved Outdated
decentral1se commented 2023-10-03 23:10:52 +00:00
Outdated
Review
Copy Link

Maybe it's time to abstract this 45 to a const in pkg/config/env.go? We're starting to use it in several places and this will probably lead to confusion at best and bugs at worst as things change over time.

Maybe it's time to abstract this `45` to a `const` in `pkg/config/env.go`? We're starting to use it in several places and this will probably lead to confusion at best and bugs at worst as things change over time.
rix commented 2023-10-04 18:13:09 +00:00
Outdated
Review
Copy Link

Done : )

Done : )
❤️ 1
failingSecrets := []string{}
for secretName := range secrets {
if len(secretName)+domainLength > 64 {
failingSecrets = append(failingSecrets, secretName)
}
}
if len(failingSecrets) > 0 {
decentral1se marked this conversation as resolved Outdated
decentral1se commented 2023-10-03 23:11:58 +00:00
Outdated
Review
Copy Link

nitpick: drop extra newline?

nitpick: drop extra newline?
rix commented 2023-10-04 18:10:55 +00:00
Outdated
Review
Copy Link

Think I've got this one now but let me know if I misinterpreted which line you were talking about : )

Think I've got this one now but let me know if I misinterpreted which line you were talking about : )
❤️ 1
failedSecretsString := strings.Join(failingSecrets, ", ")
return fmt.Errorf("The following secrets are too long to work with the domain name %s, change their length or use a shorter domain name: %s", domain, failedSecretsString)
}
return nil
decentral1se marked this conversation as resolved Outdated
decentral1se commented 2023-10-03 23:27:36 +00:00
Outdated
Review
Copy Link

Maybe 64 can also go in pkg/config/env.go while we're at it?

Maybe `64` can also go in `pkg/config/env.go` while we're at it?
rix commented 2023-10-04 18:11:04 +00:00
Outdated
Review
Copy Link

Done

Done
❤️ 1
}
decentral1se marked this conversation as resolved Outdated
decentral1se commented 2023-10-03 23:32:42 +00:00
Outdated
Review
Copy Link

I think it would be simpler to just error out here. Also a simpler/clearer error message to write?

return fmt.Errorf("%s is too long (> 64 chars when combined with %s)", secretName, sanitisedAppName)
I think it would be simpler to just error out here. Also a simpler/clearer error message to write? ```go return fmt.Errorf("%s is too long (> 64 chars when combined with %s)", secretName, sanitisedAppName) ```
rix commented 2023-10-04 18:12:57 +00:00
Outdated
Review
Copy Link

I've made it drop out after the first error now but I guess the side effect is that if there's more than one long secret it will be a bit of a whack-a-mole to find them all but I guess it's a pretty rare occurence so the trade off for cleaner code is probably worth it : )

I've made it drop out after the first error now but I guess the side effect is that if there's more than one long secret it will be a bit of a whack-a-mole to find them all but I guess it's a pretty rare occurence so the trade off for cleaner code is probably worth it : )
🎉 1