kev-458 remove label validation by mangas · Pull Request #482 · appvia/tako

mangas · 2021-04-23T10:56:54Z

No description provided.

ezodude

Left some questions/comments.

All in all a great step forward.

ezodude · 2021-04-26T06:12:37Z

pkg/kev/converter/kubernetes/helpers_test.go

 func assertLog(level logrus.Level, message string, fields map[string]string) {
 	Expect(hook.LastEntry().Level).To(Equal(level))
-	Expect(hook.LastEntry().Message).To(Equal(message))
+	Expect(cmp.Diff(hook.LastEntry().Message, message)).To(BeEmpty())


Curious on why you had to diff here?

When I was investigating I couldn't see the whole sentence, just the small difference and it is not useful. This will basically show the 2 sentences in full so it';s easier to understand what changed

ezodude · 2021-04-26T06:15:56Z

pkg/kev/converter/kubernetes/project_service.go

-	}
-
-	return config.DefaultReplicaNumber
+	return int32(p.K8SConfig.Workload.Replicas)


That's great, but should probably guard against values passed as strings too? As it stands it'll fail to extract replicas from the following config:

x-k8s: workload: replicas: "10"

Would be good to either support that also, OR handle the error if value is not as expected.

This will not pass validation because it won't be the right type. At this point we're operating on top of K8SConfiguration which is a struct with defined types. So passing replicas: "10" would result in either a parsing error or a validation error. In either case it is detected before it gets to project_service

ezodude · 2021-04-26T06:27:02Z

pkg/kev/converter/kubernetes/project_service_test.go


 	Describe("enabled", func() {
-
 		When("component toggle label is set to Truthy value", func() {


I think this may have to be reworded as we were previously working with string conversion.

I guess that's still OK, as the value you can pass in the struct can be anything so good to ensure it's truthy. But since we switched from enabled to the reverse of that some text will need to change for sure.

ezodude · 2021-04-26T06:36:29Z

pkg/kev/converter/kubernetes/project_service_test.go

-				labels = composego.Labels{
-					config.LabelWorkloadType: projectWorkloadType,
-				}
+			JustBeforeEach(func() {


No sure why we started using JustBeforeEach?

because we need to change the extensions and in this case the tests needs to override the creation of the project service since it can't be updated easily

I see.

Personally, to keep spec files simple, I try to only use just one JustBeforeEach for the entire spec file. This keeps things readable and easier to understand.

Do you think multiple JustBeforeEach statements are code small signalling that we need to split this test file into smaller test files?

Yep, I think so for sure, don't think this is the right time to do it though because of the big refactor in progress but I will definitely add it to the list

ezodude · 2021-04-26T06:36:39Z

pkg/kev/converter/kubernetes/project_service_test.go

-				BeforeEach(func() {
-					labels = composego.Labels{
-						config.LabelWorkloadRestartPolicy: policy,
+				JustBeforeEach(func() {


No sure why we started using JustBeforeEach?

because we need to change the extensions and in this case the tests needs to override the creation of the project service since it can't be updated easily

See #482 (comment)

ezodude · 2021-04-26T06:38:14Z

pkg/kev/services.go

 func newServiceConfig(s composego.ServiceConfig) (ServiceConfig, error) {
 	config := ServiceConfig{Name: s.Name, Labels: s.Labels, Environment: s.Environment, Extensions: s.Extensions}
-	return config, config.validate()
+	return config, nil


Do we still need to return an error?

I would prefer keeping it just because updating the api requires more changes. I think this kind of cleanup should be done afterwards to minimize conflicts with master

marcinc

A few comments

marcinc · 2021-04-26T09:15:16Z

pkg/kev/config/extensions.go

-	default:
-		return RestartPolicyAlways
+	if svc.Deploy != nil && svc.Deploy.RestartPolicy != nil {
+		policy = svc.Deploy.RestartPolicy.Condition


We shouldn't be returning a literal value from compose here. Previously we were mapping that value to the expected restart policy but this code was removed above.. As it stands this func will be returning mixed values.

Unless this func is meant to return the literal value extracted from compose (if present)? In which case it'd be the extracted value or an empty string, so no priming policy with RestartPolicyAlways needed. Does the switch statement need to be moved somewhere else?

This is the bit where we're moving validation to a different place. The default value used to be handled by project_service but now that doesn't make much sense since this configuration should be responsible for that part. The code as it preserves the functionality since the tests are still covering the same behaviour

marcinc · 2021-04-26T09:21:22Z

pkg/kev/converter/kubernetes/project_service.go

-	}
-
-	return config.DefaultReplicaNumber
+	return int32(p.K8SConfig.Workload.Replicas)


That's great, but should probably guard against values passed as strings too? As it stands it'll fail to extract replicas from the following config:

x-k8s: workload: replicas: "10"

Would be good to either support that also, OR handle the error if value is not as expected.

marcinc · 2021-04-26T09:24:10Z

pkg/kev/converter/kubernetes/project_service.go

 			workloadType)
 	}

-	return workloadType


I guess no need to change the return value here?

I reverted this. to use workloadType, this was left because of copy paste. I brought back the logging but forgot to change this last return

marcinc · 2021-04-26T09:38:15Z

pkg/kev/converter/kubernetes/project_service_test.go

 		})

-		When("component toggle label is set to any string value not representing a boolean value (one of: 1, t, T, TRUE, true, True, 0, f, F, FALSE, false, False.)", func() {
+		When("component toggle extension to set false value", func() {


The configuration map may contain values that aren't bool.. eg. "true" , 1, TRUE. It'd make sense to respect and handle those "truthy" values I think.

again, we no longer look at maps, we only look at the configuration struct which is type safe so if those values are set wrong they will be either caught by marshalling or validation, this is not the place to check for any of it.

marcinc · 2021-04-26T09:42:31Z

pkg/kev/converter/kubernetes/project_service_test.go

 			})

 			It("will use a replica number as specified in deploy block", func() {
+				Expect(projectService.Deploy.Replicas).NotTo(BeNil())


Maybe best to assert that the number of replicas is as expected here?

This check is just to ensure that the value set in BeforeEach is being picked up, the following assertion verifies the value so I don't think we need to make this more specific IMO, but if you want I can change it

marcinc · 2021-04-26T09:49:35Z

pkg/kev/converter/kubernetes/project_service_test.go

 				})

 				It("warns the user and defaults restart policy to `Always`", func() {
+					Expect(projectService.K8SConfig.Workload.RestartPolicy).To(Equal(policy))


Are we expecting workload RestartPolicy defined in x-k8s as "unless-stopped" here i.e. compose notation? or one of the values we defined for it in label based configuration? See: https://github.com/appvia/kev/blob/master/docs/reference/config-params.md#kevworkloadrestart-policy

k8s should hold whatever is inferred from compose and then merged with extensions content. So both cases can happen.

OK, I guess we just need to ensure that what we derive from docker compose is translated into values we want the end user to see/tweak. Currently we let the user to adjust configuration based on the config params we defined - see link above.

And these are the only values we should be populating in the projectService.K8SConfig.Workload.RestartPolicy.

The spec above expects that the value of projectService.K8SConfig.Workload.RestartPolicy is unless-stopped which I don't think is correct as it should be one of allowed values: Always, OnFailure, Never.

so if I understand correctly, you're proposing we move this to the config.K8SCfgFromCompose?

marcinc · 2021-04-26T09:56:09Z

pkg/kev/kev_reconcile_test.go

 			ReadinessProbe: config.DefaultReadinessProbe(),
 			Type:           config.DefaultWorkload,
 			Replicas:       config.DefaultReplicaNumber,
+			RestartPolicy:  "always",


This should be config.DefaultRestartPolicy I reckon.

config.DefaultRestartPolicy is "Always" and the specific code from project_service was setting "always" so i left it as it was to avoid adding to the changes. It's definitely something that can be improved later

marcinc · 2021-04-26T15:21:21Z

pkg/kev/config/extensions.go

+			"restart-policy": policy,
+		}, "Restart policy 'unless-stopped' is not supported, converting it to 'always'")
+
+		policy = "always"


OK, this function now returns what has been defined in the docker compose around restart policy, however, we prime the value of policy in the first statement to be config.RestartPolicyAlways which is a Kev config parameter value. Perhaps change it to literal string "always" instead?

ezodude

👍 Looks good - I'll defer to @marcinc for the final approval (he understands this bit of the code more than I do 😉).

marcinc

LG. We can address outstanding elements in the follow up PRs.

* kev-458 remove label validation

* 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)

kev-458 remove label validation

3c8a404

mangas marked this pull request as draft April 23, 2021 10:56

kev-458 remove label validation

8ca7ce5

mangas marked this pull request as ready for review April 23, 2021 15:38

ezodude requested review from ezodude and marcinc April 26, 2021 04:59

ezodude reviewed Apr 26, 2021

View reviewed changes

marcinc reviewed Apr 26, 2021

View reviewed changes

mangas added 2 commits April 26, 2021 12:23

code review

c04386f

fix restart policy

f3f823f

marcinc reviewed Apr 26, 2021

View reviewed changes

fix restart policy

803019f

ezodude reviewed Apr 27, 2021

View reviewed changes

marcinc approved these changes Apr 27, 2021

View reviewed changes

mangas merged commit d30ea49 into labels-to-extension Apr 27, 2021

mangas deleted the kev-458-remove-label-validation branch April 27, 2021 12:42

ezodude pushed a commit that referenced this pull request May 27, 2021

kev-458 remove label validation (#482)

784fe35

* kev-458 remove label validation

ezodude pushed a commit that referenced this pull request Jun 4, 2021

kev-458 remove label validation (#482)

06d40d3

* kev-458 remove label validation


		Describe("enabled", func() {

		When("component toggle label is set to Truthy value", func() {

Conversation

mangas commented Apr 23, 2021

Uh oh!

ezodude left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcinc Apr 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcinc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcinc Apr 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

marcinc Apr 26, 2021 •

edited

Loading

marcinc Apr 26, 2021 •

edited

Loading