kev-458 Render probe info from extensions#459
Conversation
ezodude
left a comment
There was a problem hiding this comment.
Yay. Love the simplicity and how all the value conversion / checks have been removed.
Left a couple of comments.
| } | ||
|
|
||
| // @step add PVCs to objects | ||
| if pvcs != nil { |
There was a problem hiding this comment.
What changed to allow removing this check?
There was a problem hiding this comment.
nil is the zero value for slices so for range works on them it just doesn't iterate. No need to protect with a nil check
There was a problem hiding this comment.
| } | ||
|
|
||
| // @step add ConfigMaps to objects | ||
| if cms != nil { |
There was a problem hiding this comment.
What changed to allow removing this check?
There was a problem hiding this comment.
same as the one before
| "github.com/appvia/kev/pkg/kev/config" | ||
| ) | ||
|
|
||
| func LivenessProbeToV1Probe(lp config.LivenessProbe) (*v1.Probe, error) { |
There was a problem hiding this comment.
A small note for future PR perhaps, the k8s api version will eventually change so I'd avoid embedding V1 in the function name here and below.
| TimeoutSeconds: 10, | ||
| PeriodSeconds: 10, | ||
| InitialDelaySeconds: 10, | ||
| PeriodSeconds: 60, |
There was a problem hiding this comment.
I wonder why the values for period and initial delay have changed?
There was a problem hiding this comment.
because they are not being read from the compose side yet, so these are the default for now, they will break once compose is read into extensions rather than using defaults
| Expect(err).To(MatchError("Health check misconfigured")) | ||
| _, err := projectService.LivenessProbe() | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| // Expect(err).To(MatchError("Health check misconfigured")) |
There was a problem hiding this comment.
The commented out bits can go I guess?
There was a problem hiding this comment.
This needs to be brought back when #460 is completed. Right now the validation is not completed so this doesn't fail but it should.
| "Health check misconfigured", | ||
| map[string]string{}, | ||
| ) | ||
| // assertLog(logrus.ErrorLevel, |
There was a problem hiding this comment.
Do we need the commented out bits still?
There was a problem hiding this comment.
yes, same as previous comments
| }) | ||
|
|
||
| Describe("livenessProbeCommand", func() { | ||
| When("defined via labels", func() { |
There was a problem hiding this comment.
There is a bunch of probe related tests being removed in this PR that set the specific contexts around various probe parameters being supplied via configuration (previously labels and now by extensions) and cases for when custom configuration isn't explicitly specified. Do we have the coverage of those things still?
There was a problem hiding this comment.
Yes, we now start with defaults and merge in the set configuration. There are some bits missing in terms of validation (#460) for example 0 values and such but absent values are no longer a problem (and is tested in the config package), same as wrong types
| }) | ||
| }) | ||
|
|
||
| Context("and no port label", func() { |
There was a problem hiding this comment.
It's not a label anymore so let's change the wording here too.
There was a problem hiding this comment.
I'll review all of this once we have values being read and labels completely removed
| }) | ||
| }) | ||
| }) | ||
| // Describe("livenessProbeCommand", func() { |
There was a problem hiding this comment.
Are these commented out specs something we want to keep?
There was a problem hiding this comment.
yes, we still want to implement them later again but right now there is no point changing them just to change it back later IMO
* Revert "kev-458 Render probe info from extensions (#459)" This reverts commit 7cfd284. * Revert "kev-449 overlay validation (#451)" This reverts commit c6c4ec0. * Revert "add extensions to init (#439)" This reverts commit 539480d. * Revert "add extension support (#432)" This reverts commit d694204. Co-authored-by: marcinc <marcin.ciszak@appvia.io>
* add extension support (#432) * add extension support * add extensions to init (#439) * kev-449 overlay validation (#451) * kev-458 Render probe info from extensions (#459) * Latest from master. * kev-458 remove label validation (#482) * kev-458 remove label validation * Kev 458 cleanup moved extensions (#483) * kev-458 cleanup moved extensions * remove unused code * kev-458 move imagepull labels (#498) * kev-458 move resource (#499) * kev-458 move cpu (#500) * Bug fix - can now use correct workload type during init and render. (#504) * Extract volume settings into a volumes K8s extension (#502) * Enable volumes reconciliation for extensions. (#506) * Render volumes using extension settings (#508) * Remove volume labels use (#509) * Moved the autoscale labels to extensions. (#514) * Move the pod-security labels to extensions. (#516) * Move the rolling update max surge and service account name to extensions. (#519) * Move the service labels to extensions (#521) * Remove services labels use. (#525) * Turn Restart Policy string value sets into enums (#533) * Turn Workload Type and Service Type value sets into enums (#534) * Minify Extensions (#536) * Set correct livenessProbe success threshold (#548) * Update the documentation to use extensions instead of labels. (#547) * Handle ingress with kubernetes cluster defaults (#552)
Part 1 of x-k8s extensions - probe rendering
Closes #458