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: Allow script and container image to be set in templateDefault. Fixes #9633 #10784

Merged
merged 1 commit into from
Apr 11, 2023

Conversation

RoryDoherty
Copy link
Contributor

@RoryDoherty RoryDoherty commented Mar 30, 2023

Made changes to validation to check if the script/container Image has been set in templateDefaults

Testing
Added unit tests to verify these situations

Also created the following via the locally deployed UI and then submitted both via the UI and they were successful:

apiVersion: argoproj.io/v1alpha1
kind: WorkflowTemplate
metadata:
  generateName: demo-
spec:
  entrypoint: demo
  templateDefaults:
    script:
      image: alpine:latest
      command: [sh]
      workingDir: "/src"
  templates:
    - name: demo
      script:
        source: |
          pwd

and a cron workflow

apiVersion: argoproj.io/v1alpha1
kind: CronWorkflow
metadata:
  generateName: demo-cron
spec:
  schedule: "00 4 * * 2"
  timezone: "UTC"
  workflowMetadata:
    generateName: demo-cron-test-
  workflowSpec:
    entrypoint: demo
    templateDefaults:
      script:
        image: alpine:latest
        command: [sh]
        workingDir: "/src"
    templates:
      - name: demo
        script:
          source: |
            pwd

Fixes #9633

@RoryDoherty RoryDoherty force-pushed the templateDefault-fix branch 2 times, most recently from 5a2ca95 to 06ecf8f Compare March 30, 2023 14:51
@RoryDoherty RoryDoherty marked this pull request as ready for review March 30, 2023 16:15
Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also add a test for the functionality, e.g. e2e test?

@RoryDoherty
Copy link
Contributor Author

Could you also add a test for the functionality, e.g. e2e test?

@terrytangyuan sure no problem, do you have a particular suite or file you would like it added to?

@terrytangyuan
Copy link
Member

@RoryDoherty RoryDoherty force-pushed the templateDefault-fix branch 3 times, most recently from 8d8dff6 to 9fde944 Compare April 11, 2023 17:22
@terrytangyuan terrytangyuan enabled auto-merge (squash) April 11, 2023 17:30
auto-merge was automatically disabled April 11, 2023 17:52

Head branch was pushed to by a user without write access

@RoryDoherty RoryDoherty force-pushed the templateDefault-fix branch 2 times, most recently from d977286 to 12e2715 Compare April 11, 2023 18:52
@terrytangyuan terrytangyuan merged commit b87bdcf into argoproj:master Apr 11, 2023
21 checks passed
terrytangyuan pushed a commit that referenced this pull request May 25, 2023
…ixes #9633 (#10784)

Signed-off-by: Rory Doherty <rory@tigera.io>
JPZ13 pushed a commit to pipekit/argo-workflows that referenced this pull request Jul 4, 2023
dpadhiar pushed a commit to dpadhiar/argo-workflows that referenced this pull request May 9, 2024
…ixes argoproj#9633 (argoproj#10784)

Signed-off-by: Rory Doherty <rory@tigera.io>
Signed-off-by: Dillen Padhiar <dillen_padhiar@intuit.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

temaplateDefaults: container/script: image does nothing
3 participants