-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Adds Workload interface to access PodSpec for all workload types #25
Conversation
…types Provide a consistent interface Workload and implementations that can access the PodSpec and other common k8s data structure for Pod, Deployment, StatefulSet, DaemonSet, Job, and CronJob.
internal/workload.go
Outdated
"sigs.k8s.io/controller-runtime/pkg/client" | ||
) | ||
|
||
// Workload interface a standard way to access the pod definition for the |
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.
Workload is a standard way...?
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.
fixed
internal/workload.go
Outdated
// necessary configuration and other details before it starts, or if the | ||
// configuration changes. | ||
type Workload interface { | ||
GetPodSpec() corev1.PodSpec |
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.
This should just be PodSpec
. Cf. https://go.dev/doc/effective_go#Getters
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.
Fixed.
internal/workload.go
Outdated
type Workload interface { | ||
GetPodSpec() corev1.PodSpec | ||
SetPodSpec(spec corev1.PodSpec) | ||
GetObject() client.Object |
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.
Same thing here: Object
will do.
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.
fixed.
internal/workload_test.go
Outdated
}, | ||
} | ||
|
||
t.Parallel() |
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.
Unnecessary until we have tests that aren't running fast.
internal/workload_test.go
Outdated
} | ||
|
||
// mustMakeWorkload is shorthand to create workload test inputs | ||
func mustMakeWorkload(kind string, ns string, name string, l ...string) Workload { |
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.
This should take a *testing.T
so you can fail the test rather than kill the process.
internal/workload_test.go
Outdated
desc: "match kind, namespace, and labels", | ||
sel: cloudsqlapi.WorkloadSelectorSpec{ | ||
Kind: "Pod", | ||
Name: "", |
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.
Remember the zero value is already an empty string.
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.
fixed
internal/workload_test.go
Outdated
{ | ||
desc: "match namespace, and labels", | ||
sel: cloudsqlapi.WorkloadSelectorSpec{ | ||
Kind: "", |
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.
Ditto here and below.
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.
fixed
internal/workload_test.go
Outdated
{ | ||
desc: "match namespace, and label expression", | ||
sel: cloudsqlapi.WorkloadSelectorSpec{ | ||
Kind: "", |
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.
and here too
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.
fixed
internal/workload_test.go
Outdated
case "DaemonSet": | ||
v = &DaemonSetWorkload{DaemonSet: &appsv1.DaemonSet{TypeMeta: metav1.TypeMeta{Kind: "DaemonSet"}}} | ||
default: | ||
log.Fatalf("Workload kind %s not supported", kind) |
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.
This should be t.Fatalf
.
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.
t is not in this context.
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.
But it should be ;-)
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.
fixed
internal/workload_test.go
Outdated
v.GetObject().SetName(name) | ||
if len(l) > 0 { | ||
if len(l)%2 != 0 { | ||
log.Fatalf("labels list must have an even number of elements") |
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.
t.Fatalf
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.
same
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.
fixed
Provide a consistent interface Workload and implementations that can access the PodSpec and other common k8s data structure for Pod, Deployment, StatefulSet, DaemonSet, Job, and CronJob.