Skip to content
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

fix(webhook): Add default values for parallelism.completionStrategy #88

Merged
merged 1 commit into from
Jun 3, 2022

Conversation

irvinlim
Copy link
Member

@irvinlim irvinlim commented Jun 3, 2022

Bug Reproduction

Trying to apply the following YAML results in not being able to save:

apiVersion: execution.furiko.io/v1alpha1
kind: JobConfig
metadata:
  name: jobconfig-parallel-sleep
spec:
  concurrency:
    policy: Forbid
  schedule:
    cron:
      expression: "H/15 * * * *"
      timezone: Asia/Singapore
    disabled: false
  template:
    spec:
      parallelism:
        withKeys:
          - "30"
          - "60"
          - "120"
      taskTemplate:
        pod:
          spec:
            containers:
              - name: job-container
                args:
                  - bash
                  - -c
                  - "echo Sleeping for ${task.index_key}; sleep ${task.index_key}"
                image: bash

The following error is displayed:

$ k apply -f jobconfig-parallel-sleep.yaml
The JobConfig "jobconfig-parallel-sleep" is invalid: spec.template.spec.parallelism.completionStrategy: Required value

Bugfix

By right, we should also add default values for JobTemplate in JobConfigSpec. The default value should be set to AllSuccessful.

@irvinlim irvinlim added kind/bug Categorizes issue or PR as related to a bug. component/execution Issues or PRs related exclusively to the Execution component (Job, JobConfig) area/webhooks Related to validating and mutating admission webhooks. labels Jun 3, 2022
@codecov
Copy link

codecov bot commented Jun 3, 2022

Codecov Report

Merging #88 (ad6a910) into main (25d2757) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #88      +/-   ##
==========================================
+ Coverage   62.87%   62.88%   +0.01%     
==========================================
  Files         200      200              
  Lines       10334    10342       +8     
==========================================
+ Hits         6497     6504       +7     
- Misses       3489     3490       +1     
  Partials      348      348              
Impacted Files Coverage Δ
pkg/execution/mutation/mutation.go 89.09% <100.00%> (+5.35%) ⬆️
...kg/execution/controllers/croncontroller/metrics.go 14.28% <0.00%> (-85.72%) ⬇️
pkg/runtime/reconciler/metrics.go 42.10% <0.00%> (-26.32%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 25d2757...ad6a910. Read the comment docs.

@irvinlim irvinlim merged commit 422fc7e into main Jun 3, 2022
@irvinlim irvinlim deleted the irvinlim/fix/default-completion-strategy branch June 3, 2022 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/webhooks Related to validating and mutating admission webhooks. component/execution Issues or PRs related exclusively to the Execution component (Job, JobConfig) kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant