feat: Add internal libraries for kubernetes names #23
Conversation
Adds a new internal/names package to the project which has functions to create safe kubernetes names of appropriate formatting and length. This will be used to format modified workload sidecar container names.
…managed by the operator.
internal/names/names.go
Outdated
| cloudsqlapi "github.com/GoogleCloudPlatform/cloud-sql-proxy-operator/api/v1alpha1" | ||
| ) | ||
|
|
||
| const maxNameLen = 63 |
There was a problem hiding this comment.
nit:
const (
maxNameLen = 63
hashLen = 8
dividerLen = 2
)Also what are these numbers?
There was a problem hiding this comment.
I see 63 characters is the max length imposed by Kubernetes. A comment to that effect here would be good. Or if these are only used once, inlining them with a comment would be fine too.
There was a problem hiding this comment.
I moved these into SafePrefixedName, merged them into one declaration, and added some comments.
internal/names/names.go
Outdated
| nameSuffix := instName[len(instName)-truncateLen:] | ||
| checksum := hash(instName) | ||
| return strings.ToLower(fmt.Sprintf("%s%s-%s-%x", prefix, namePrefix, nameSuffix, checksum)) | ||
| } else { |
There was a problem hiding this comment.
Here's another unnecessary else.
internal/names/names.go
Outdated
| } | ||
|
|
||
| // hash simply returns the checksum for a string | ||
| func hash(s string) uint32 { |
There was a problem hiding this comment.
Could this take a slice of bytes instead of a string? It's more common to work on bytes and convert to strings when a user is going to see it.
internal/names/names_test.go
Outdated
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| ) | ||
|
|
||
| func TestSafeDnsLabel(t *testing.T) { |
There was a problem hiding this comment.
Fixed. it shouldn't be called "DNS" anymore.
internal/names/names_test.go
Outdated
| ) | ||
|
|
||
| func TestSafeDnsLabel(t *testing.T) { | ||
| t.Parallel() |
There was a problem hiding this comment.
Is this necessary? These tests are already pretty fast.
internal/names/names_test.go
Outdated
|
|
||
| } | ||
|
|
||
| // TestContainerName container names are a public |
There was a problem hiding this comment.
full sentences please. But I'm not sure this comment adds much given the docs are already good.
internal/names/names_test.go
Outdated
|
|
||
| } | ||
|
|
||
| // TestVolumeName container names are a public |
internal/names/names_test.go
Outdated
| ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: namespace}, | ||
| Spec: cloudsqlapi.AuthProxyWorkloadSpec{ | ||
| Workload: cloudsqlapi.WorkloadSelectorSpec{ | ||
| Kind: "", |
There was a problem hiding this comment.
The zero value is already empty string. This is unnecessary.
internal/names/names_test.go
Outdated
|
|
||
| } | ||
|
|
||
| func mustMakeCsql(name string, namespace string) *cloudsqlapi.AuthProxyWorkload { |
There was a problem hiding this comment.
Must implies there's an error you're failing on. But I don't see any error. So this could just be authProxyWorkload.
There was a problem hiding this comment.
renamed to authProxyWorkload()
| Workload: cloudsqlapi.WorkloadSelectorSpec{ | ||
| Selector: &metav1.LabelSelector{ | ||
| MatchLabels: map[string]string{"app": "hello"}, | ||
| MatchExpressions: nil, |
There was a problem hiding this comment.
Don't worry about setting the zero value. The language does that for you.
2 participants
Adds a new internal/names package to the project which has functions to create
kubernetes-safe names of appropriate formatting and length. This will
be used to format the names of containers and volumes that are added to workloads.