Add support for Job and CronJob (closes #86 and #212) #411

mbarrien · 2019-04-23T06:58:45Z

This PR adds Terraform resources for Kubernetes Job and CronJob objects, and closes #86 and #212. It is mostly cherry picks from the sl1pm4t fork, using contributions from @srhaber, @sl1pm4t and @bassrock, and rebased against the head of master (currently just past 1.6.2). The licensing on the other branch should be compatible for cherry-picking here.

Although the commits are ostensibly cherry-picks, I've had to modify some of them for compatibility with the original branch. For followers of the sl1pm4t fork, these changes are:

Does not copy the top level metadata to the pod spec's metadata
Uses expandPodTemplate from the main branch rather than expandPodTemplateSpec from the fork
Does not hard code job spec restart policy to OnFailure
Forces job to use BatchV1beta1 api, removing support for BatchV2alpha1 api
Restore ability to patch kubernetes_job resources

alexsomesan · 2019-04-24T14:12:18Z

.travis.yml

@@ -5,6 +5,7 @@ services:
 language: go
 go:
  - "1.11.x"
+go_import_path: github.com/terraform-providers/terraform-provider-kubernetes


Any particular reason why this was needed?

alexsomesan · 2019-04-24T14:38:05Z

kubernetes/resource_kubernetes_cron_job.go

+
+	out, err := conn.BatchV1beta1().CronJobs(metadata.Namespace).Create(&job)
+	if err != nil {
+		return err


Mind wrapping this error with a message mentioning the Create operation failed?

Suggested change

return err

return fmt.Errorf("Failed to create CronJob: %s", err)

alexsomesan · 2019-04-24T14:38:40Z

kubernetes/resource_kubernetes_cron_job.go

+
+	out, err := conn.BatchV1beta1().CronJobs(namespace).Update(cronjob)
+	if err != nil {
+		return err


Error wrapping.

alexsomesan · 2019-04-24T15:34:48Z

kubernetes/resource_kubernetes_cron_job.go

+
+	namespace, name, err := idParts(d.Id())
+	if err != nil {
+		return err


Error wrapping.

alexsomesan · 2019-04-24T15:34:56Z

kubernetes/resource_kubernetes_cron_job.go

+	job, err := conn.BatchV1beta1().CronJobs(namespace).Get(name, metav1.GetOptions{})
+	if err != nil {
+		log.Printf("[DEBUG] Received error: %#v", err)
+		return err


Error wrapping.

alexsomesan · 2019-04-24T16:16:18Z

kubernetes/schema_cron_job_spec.go

+		"schedule": {
+			Type:     schema.TypeString,
+			Required: true,
+			//ValidateFunc: validate, TODO: validate cron syntax..


What's the plan here?
One option would be validation.ValidateRegexp(...) paired with something like http://regexlib.com/REDetails.aspx?regexp_id=2580&AspxAutoDetectCookieSupport=1

What about reusing cron.ParseStandard() that the kubernetes API server uses for validation?

Even better idea! :)

alexsomesan · 2019-04-24T16:18:23Z

kubernetes/schema_cron_job_spec.go

+		"failed_jobs_history_limit": {
+			Type:        schema.TypeInt,
+			Optional:    true,
+			Default:     "1",


This attribute is TypeInt. I presume the Default value should be an int literal.

alexsomesan · 2019-04-24T16:18:24Z

kubernetes/schema_cron_job_spec.go

+		"failed_jobs_history_limit": {
+			Type:        schema.TypeInt,
+			Optional:    true,
+			Default:     "1",


This attribute is TypeInt. I presume the Default value should be an int literal.

alexsomesan

Thanks for porting these resources over.

I've left some comments around the code. As usual, I can help and make those changes and push on top of the PR if that's ok with you.

alexsomesan · 2019-06-17T13:06:47Z

kubernetes/resource_kubernetes_cron_job.go

+
+	log.Printf("[INFO] Updating cron job %s: %s", d.Id(), cronjob)
+
+	out, err := conn.BatchV1beta1().CronJobs(namespace).Update(cronjob)


The Update() client-go operation is doing a complete replacing of the resource, rather then mutating only the fields present in the Terraform diff.

This is generally not the behaviour Terraform users expect when they see a diff with updates, since this wholesale-replace behaviour is modeled in Terraform via a Delete/Create sequence.

To achieve the expected behaviour of in-place updating specific attributes as per Terraform diff, one has to model this operation using d.HasChange(..) to extract changes for the Terraform diff and build a set of JSON-Patch actions to be passed to the Patch(..) operation of the client-go.

Alternatively, if the resource doesn't support in-place update, then the Update operation on the provider resource should just not be implemented.

I notice the Job resource also introduced in this PR does make proper use of patching, so it'd be great to have them both aligned.

alexsomesan · 2019-06-17T13:58:57Z

kubernetes/resource_kubernetes_cron_job.go

+	log.Printf("[INFO] Received cron job: %#v", job)
+
+	// Remove server-generated labels unless using manual selector
+	if _, ok := d.GetOk("spec.0.manual_selector"); !ok {


This dot-syntax type of attribute referencing in d.Get(..) is on a deprecation path and not guaranteed to be supported in TF 0.12 and beyond. It'd be better to move this operation to its appropriate flatten func.

alexsomesan · 2019-06-17T22:34:58Z

kubernetes/resource_kubernetes_cron_job.go

+	log.Printf("[INFO] Deleting cron job: %#v", name)
+	err = conn.BatchV1beta1().CronJobs(namespace).Delete(name, nil)
+	if err != nil {
+		return err


Error wrapping.

alexsomesan · 2019-06-17T22:35:46Z

kubernetes/resource_kubernetes_cron_job_test.go

+	})
+}
+
+func TestAccKubernetesCronJob_extra(t *testing.T) {


Would be nice to expand extra to a more descriptive term.

alexsomesan · 2019-06-17T22:38:26Z

kubernetes/resource_kubernetes_cron_job_test.go

+
+func testAccKubernetesCronJobConfig_basic(name string) string {
+	return fmt.Sprintf(`
+resource "kubernetes_cron_job" "test" {


This inline HCL should be formatted similarly to what terraform fmt would produce.
Also, it needs to be Terraform 0.12 compatible (can be obtained by passing it to terraform 0.12upgrade).

alexsomesan · 2019-06-17T23:01:46Z

kubernetes/resource_kubernetes_job.go

+	log.Printf("[INFO] Updating job %s: %#v", d.Id(), ops)
+
+	out, err := conn.BatchV1().Jobs(namespace).Patch(name, pkgApi.JSONPatchType, data)
+	// out, err := conn.BatchV1().Jobs(namespace).Update(&job)


alexsomesan · 2019-06-17T23:11:35Z

kubernetes/resource_kubernetes_job.go

+	log.Printf("[INFO] Deleting job: %#v", name)
+	err = conn.BatchV1().Jobs(namespace).Delete(name, nil)
+	if err != nil {
+		return err


Error wrapping.

alexsomesan · 2019-06-17T23:16:28Z

kubernetes/structure_cron_job.go

+	if in.StartingDeadlineSeconds != nil {
+		att["starting_deadline_seconds"] = int64(*in.StartingDeadlineSeconds)
+	} else {
+		att["starting_deadline_seconds"] = 0


The else block is not necessary. Zero is the default value for unset TypeInt. Also, there is a Default attribute on it's schema already.

alexsomesan · 2019-06-17T23:18:18Z

kubernetes/structure_cron_job.go

+	if in.SuccessfulJobsHistoryLimit != nil {
+		att["successful_jobs_history_limit"] = int32(*in.SuccessfulJobsHistoryLimit)
+	} else {
+		att["successful_jobs_history_limit"] = 3


Not needed. This attribute already has a Default in its schema.

alexsomesan · 2019-06-17T23:18:41Z

kubernetes/structure_cron_job.go

+	if in.FailedJobsHistoryLimit != nil {
+		att["failed_jobs_history_limit"] = int(*in.FailedJobsHistoryLimit)
+	} else {
+		att["failed_jobs_history_limit"] = 1


Not needed. This attribute already has a Default in its schema.

…alues not honoured.

Previously updates were a NoOp.

Improve CronJob tests. Cron/Job spec template fixes.

alexsomesan · 2019-06-24T13:47:03Z

I completed the remaining items to get this change in mergeable shape.
Added docs and extended the test cases to reflect all attributes.
Fix issues with immutable attribute errors on updates.

alexsomesan

CI is green.

ghost added the size/XXL label Apr 23, 2019

mbarrien force-pushed the job branch from b967734 to c321ccf Compare April 23, 2019 16:47

clairefinnie mentioned this pull request Apr 30, 2019

Terraform Provider upgrade sailthru/terraform-provider-kubernetes#1

Open

mbarrien force-pushed the job branch from c321ccf to a4ba45c Compare May 11, 2019 00:14

pdecat mentioned this pull request May 13, 2019

Template spec restart_policy unsupported value "Never" #435

Closed

dustinblackman mentioned this pull request May 24, 2019

Feature Request: CronJob resource. #212

Closed

alexsomesan added this to the 1.8.0 milestone Jun 7, 2019

alexsomesan reviewed Jun 17, 2019

View reviewed changes

srhaber and others added 18 commits June 19, 2019 15:13

Add kubernetes_job resource.

321edb6

Ignore controller-uid and job-name labels.

51ca9ae

Fix beta resources for v4 client-go

ac5f5d7

Get job resource tests working, disable Updates for now.

8ef706c

Fix tests broken by API version update

4dd3ba4

WIP - update resource API versions to v1

b6a9819

Complete out CronJob schema and add tests

b57cf57

Add backoff_limit attr to Job / CronJob

93f8a8d

Enable update for job to pass tests

90f15ec

First pass at adding cron jobs

a8207ee

Making pull request modifications

b5dcd47

Complete out CronJob schema and add tests

2a45f60

CronJob: StartingDeadlineSeconds + SuccessfulJobHistoryLimit config v…

38d2829

…alues not honoured.

Add backoff_limit attr to Job / CronJob

438debc

Implement Update for cron_job resources.

1f5a782

Previously updates were a NoOp.

Handle forked path in Travis

f79374d

CronJob: adapt PodTemplate for re-use in CronJob and fix tests.

22f458f

Improve CronJob tests. Cron/Job spec template fixes.

Revert .travis.yml back to master

af3b4f0

alexsomesan force-pushed the job branch from a4ba45c to af3b4f0 Compare June 21, 2019 20:00

go mod vendor

60eb0ea

ghost added the dependencies label Jun 21, 2019

Add docs for Jobs and CronJobs

a0b433b

ghost added the documentation label Jun 24, 2019

alexsomesan approved these changes Jun 24, 2019

View reviewed changes

alexsomesan merged commit 71bd6b0 into hashicorp:master Jun 24, 2019

mbarrien deleted the job branch October 21, 2019 06:26

ghost locked and limited conversation to collaborators Apr 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Job and CronJob (closes #86 and #212) #411

Add support for Job and CronJob (closes #86 and #212) #411

mbarrien commented Apr 23, 2019 •

edited

Loading

alexsomesan Apr 24, 2019

alexsomesan Apr 24, 2019 •

edited

Loading

alexsomesan Apr 24, 2019

alexsomesan Apr 24, 2019

alexsomesan Apr 24, 2019

alexsomesan Apr 24, 2019

pdecat Jun 18, 2019

alexsomesan Jun 18, 2019

alexsomesan Apr 24, 2019

alexsomesan Apr 24, 2019

alexsomesan left a comment

alexsomesan Jun 17, 2019

alexsomesan Jun 17, 2019

alexsomesan Jun 17, 2019

alexsomesan Jun 17, 2019

alexsomesan Jun 17, 2019

alexsomesan Jun 17, 2019

alexsomesan Jun 17, 2019

alexsomesan Jun 17, 2019

alexsomesan Jun 17, 2019

alexsomesan Jun 17, 2019

alexsomesan commented Jun 24, 2019

alexsomesan left a comment

	return err
	return fmt.Errorf("Failed to create CronJob: %s", err)


		log.Printf("[INFO] Updating cron job %s: %s", d.Id(), cronjob)

		out, err := conn.BatchV1beta1().CronJobs(namespace).Update(cronjob)

Add support for Job and CronJob (closes #86 and #212) #411

Add support for Job and CronJob (closes #86 and #212) #411

Conversation

mbarrien commented Apr 23, 2019 • edited Loading

Choose a reason for hiding this comment

alexsomesan Apr 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexsomesan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexsomesan commented Jun 24, 2019

alexsomesan left a comment

Choose a reason for hiding this comment

mbarrien commented Apr 23, 2019 •

edited

Loading

alexsomesan Apr 24, 2019 •

edited

Loading