Job CLI deploy/list/export/remove #119

stinkyfingers · 2021-07-21T20:04:27Z

Description

Adds CLI commands job deploy, job list, job export, job remove. Job deploy only implements yaml file deployments. Spec here. Makes version, policy, and description fields optional.

I also found some small fixes that were needed for the Job CRD, noted in comments.

Try it:
make install ketch manager
./bin/manager --enable-leader-election=false --disable-webhooks (assuming you're not deploying)
./bin/ketch framework add fw
./bin/ketch job deploy job.yaml
k get jobs -n ketch-fw
k get pods -n ketch-fw
k logs <pod> - n ketch-fw pi
./bin/ketch job export myjob
./bin/ketch job list
./bin/ketch job remove myjob

example job.yaml:

name: myjob
version: v1
type: Job
framework: fw
description: "cli test job"
containers:
  - name: pi
    image: perl
    command:
      - "perl"
      - "-Mbignum=bpi"
      - "-wle"
      - "print bpi(2000)"

Fixes # 1542

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Chore (documentation addition or typo, file relocation)

Testing

New tests were added with this PR that prove my fix is effective or that my feature works (describe below this bullet)
This change requires no testing (i.e. documentation update)

Documentation

All added public packages, funcs, and types have been documented with doc comments
I have commented my code, particularly in hard-to-understand areas

Final Checklist:

I followed standard GitHub flow guidelines
I have performed a self-review of my own code
My changes generate no new warnings

Additional Information (omit if empty)

Please include anything else relevant to this PR that may be useful to know.

adds unit tests fixes job templates; fixes unit tests adds job cli tests make install includes job add job help test troubleshoot cli tests fix job help test chasing missing framework modify job create/update in deploy; chase cli test errors mv job tests to new file try go integration tests add longer wait for framework list make job cluster-level; fix up tests reset app tests reset app tests clean up tests more; fix job.deploy usage rm extra assertion

stinkyfingers · 2021-07-23T12:46:08Z

config/crd/kustomization.yaml

@@ -4,6 +4,7 @@
 resources:
 - bases/theketch.io_apps.yaml
 - bases/theketch.io_frameworks.yaml
+- bases/theketch.io_jobs.yaml


Forgot to add this during the job CRD PR #116. Makes make install install job crd.

stinkyfingers · 2021-07-23T12:46:34Z

internal/api/v1beta1/job_types.go

 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )

 // JobSpec defines the desired state of Job
 type JobSpec struct {
-	Version      string      `json:"version"`
+	Version      string      `json:"version,omitempty"`


A few more potentially empty fields.

stinkyfingers · 2021-07-23T12:47:08Z

internal/api/v1beta1/job_types.go

@@ -45,6 +45,8 @@ type JobStatus struct {

 //+kubebuilder:object:root=true
 //+kubebuilder:subresource:status
+// +kubebuilder:resource:scope=Cluster


I think job scope should be cluster level, like app. Missed this in in the job CRD PR #116.

stinkyfingers · 2021-07-23T12:48:27Z

internal/templates/job/yamls/job.yaml

@@ -7,16 +7,16 @@ metadata:
  name: {{ $.Values.job.name }}
 spec:
  {{- if $.Values.job.parallelism }}
-{{ $.Values.job.parallelism | toYaml | indent 4 }}
+  parallelism: {{ $.Values.job.parallelism }}


I botched a few template fields in the job CRD PR #116. Fixed here.

aleksej-paschenko

looks awesome

aleksej-paschenko · 2021-07-23T13:11:41Z

cmd/ketch/job_deploy.go

+	}
+
+	var job ketchv1.Job
+	err = cfg.Client().Get(ctx, types.NamespacedName{Name: spec.Name}, &job)


how about using

import ( ctrl "sigs.k8s.io/controller-runtime" ) ctrl.CreateOrUdate()

https://github.com/kubernetes-sigs/controller-runtime/blob/master/alias.go#L115
?

Cool. I didn't even know it existed...until now.

aleksej-paschenko · 2021-07-23T13:12:20Z

cmd/ketch/job_deploy_test.go

+				require.NotNil(t, err)
+				require.Equal(t, tt.wantErr, err.Error())
+				return
+			} else {


no need to use else statement.

Oops. Thanks for catching all these.

aleksej-paschenko · 2021-07-23T13:19:32Z

cmd/ketch/job_export.go

+	if err := cfg.Client().Get(ctx, types.NamespacedName{Name: options.name}, &job); err != nil {
+		return err
+	}
+	if options.filename != "" {


we use this pattern multiple times, how about having an independent function for exporting?

Good call. Refactored into the cmd/output package for the 3 export commands to use.

I talked about the whole piece of code

if options.filename != "" { f, err := output.GetOutputFile(options.filename) if err != nil { return err } defer f.Close() out = f } b, err := yaml.Marshal(job.Spec) if err != nil { return err } _, err = out.Write(b)

but it doesn't really matter, it is up to you

aleksej-paschenko · 2021-07-23T13:20:28Z

cmd/ketch/job_list.go

+func jobListNames(cfg config, nameFilter ...string) ([]string, error) {
+	jobs := ketchv1.JobList{}
+	if err := cfg.Client().List(context.TODO(), &jobs); err != nil {
+		return nil, fmt.Errorf("failed to list apps: %w", err)


should it be failed to list jobs?

Good catch. It's possible I was copy/pasting.

aleksej-paschenko · 2021-07-23T13:22:14Z

cmd/ketch/job_remove_test.go

+				require.NotNil(t, err)
+				require.Equal(t, tt.wantErr, err.Error())
+				return
+			} else {


no need to have else statement

aleksej-paschenko · 2021-07-23T13:23:16Z

cli_tests/job.sh

+
+# To run locally:
+# export KETCH_EXECUTABLE_PATH=<location of ketch binary>
+# assure you have a kubernetes cluster running w/ traefik, cert manager, etc. (see ketch getting started docs)


aleksej-paschenko · 2021-07-23T13:26:03Z

cmd/ketch/job_export_test.go

+				require.NotNil(t, err)
+				require.Equal(t, tt.wantErr, err.Error())
+				return
+			} else {


no need to use else

…utputFile func; removes unneeded 'else' clauses

aleksej-paschenko · 2021-07-23T16:50:04Z

cmd/ketch/job_export.go

+	if err := cfg.Client().Get(ctx, types.NamespacedName{Name: options.name}, &job); err != nil {
+		return err
+	}
+	if options.filename != "" {


I talked about the whole piece of code

if options.filename != "" { f, err := output.GetOutputFile(options.filename) if err != nil { return err } defer f.Close() out = f } b, err := yaml.Marshal(job.Spec) if err != nil { return err } _, err = out.Write(b)

but it doesn't really matter, it is up to you

stinkyfingers · 2021-07-23T17:40:52Z

Haha @aleksej-paschenko. Got it. It went right over my head. Refactored the whole block.

stinkyfingers force-pushed the jds/1542 branch from 6554f49 to 7e99a51 Compare July 21, 2021 20:17

stinkyfingers added the work in progress label Jul 21, 2021

stinkyfingers force-pushed the jds/1542 branch 2 times, most recently from b960a27 to 0b5fb40 Compare July 22, 2021 20:47

stinkyfingers force-pushed the jds/1542 branch from 0b5fb40 to 70fae56 Compare July 22, 2021 21:07

stinkyfingers removed the work in progress label Jul 22, 2021

stinkyfingers commented Jul 23, 2021

View reviewed changes

aleksej-paschenko reviewed Jul 23, 2021

View reviewed changes

switches job.deploy to use createOrUpdate; fixes typo; refactors GetO…

41d25e8

…utputFile func; removes unneeded 'else' clauses

aleksej-paschenko approved these changes Jul 23, 2021

View reviewed changes

refactors writeoutput harder

dae6d83

stinkyfingers merged commit 9d1a47d into main Jul 23, 2021

stinkyfingers deleted the jds/1542 branch July 23, 2021 18:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Job CLI deploy/list/export/remove #119

Job CLI deploy/list/export/remove #119

stinkyfingers commented Jul 21, 2021 •

edited

Loading

stinkyfingers Jul 23, 2021 •

edited

Loading

stinkyfingers Jul 23, 2021

stinkyfingers Jul 23, 2021 •

edited

Loading

stinkyfingers Jul 23, 2021

aleksej-paschenko left a comment

aleksej-paschenko Jul 23, 2021

stinkyfingers Jul 23, 2021

aleksej-paschenko Jul 23, 2021

stinkyfingers Jul 23, 2021

aleksej-paschenko Jul 23, 2021

stinkyfingers Jul 23, 2021

aleksej-paschenko Jul 23, 2021

aleksej-paschenko Jul 23, 2021

stinkyfingers Jul 23, 2021

aleksej-paschenko Jul 23, 2021

aleksej-paschenko Jul 23, 2021

aleksej-paschenko Jul 23, 2021

aleksej-paschenko Jul 23, 2021

stinkyfingers commented Jul 23, 2021

Job CLI deploy/list/export/remove #119

Job CLI deploy/list/export/remove #119

Conversation

stinkyfingers commented Jul 21, 2021 • edited Loading

Description

Type of change

Testing

Documentation

Final Checklist:

Additional Information (omit if empty)

stinkyfingers Jul 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stinkyfingers Jul 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aleksej-paschenko 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stinkyfingers commented Jul 23, 2021

stinkyfingers commented Jul 21, 2021 •

edited

Loading

stinkyfingers Jul 23, 2021 •

edited

Loading

stinkyfingers Jul 23, 2021 •

edited

Loading