diff --git a/.github/workflows/ci-build.yaml b/.github/workflows/ci-build.yaml index c2b4a87124a4..28ab8f626573 100644 --- a/.github/workflows/ci-build.yaml +++ b/.github/workflows/ci-build.yaml @@ -58,7 +58,7 @@ jobs: e2e-tests: name: E2E Tests runs-on: ubuntu-latest - timeout-minutes: 25 + timeout-minutes: 30 needs: [ argoexec-image ] env: KUBECONFIG: /home/runner/.kubeconfig diff --git a/test/e2e/testdata/workflow-templates/success-hook.yaml b/test/e2e/testdata/workflow-templates/success-hook.yaml new file mode 100644 index 000000000000..a0cf43b79a7f --- /dev/null +++ b/test/e2e/testdata/workflow-templates/success-hook.yaml @@ -0,0 +1,21 @@ +apiVersion: argoproj.io/v1alpha1 +kind: WorkflowTemplate +metadata: + name: hook +spec: + entrypoint: main + templates: + - name: main + container: + image: argoproj/argosay:v2 + command: ["/bin/sh", "-c"] + # To avoid flakiness, we sleep 1 second. + args: ["/bin/sleep 1; /argosay"] + + hooks: + running: + expression: workflow.status == "Running" + template: main + succeed: + expression: workflow.status == "Succeeded" + template: main diff --git a/test/e2e/workflow_template_test.go b/test/e2e/workflow_template_test.go index 50ca8e5b4e6d..519051150fa8 100644 --- a/test/e2e/workflow_template_test.go +++ b/test/e2e/workflow_template_test.go @@ -4,10 +4,12 @@ package e2e import ( + "strings" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/suite" + apiv1 "k8s.io/api/core/v1" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" "github.com/argoproj/argo-workflows/v3/pkg/apis/workflow/v1alpha1" @@ -126,6 +128,36 @@ spec: ExpectPVCDeleted() } +func (s *WorkflowTemplateSuite) TestWorkflowTemplateWithHook() { + s.Given(). + WorkflowTemplate("@testdata/workflow-templates/success-hook.yaml"). + Workflow(` +metadata: + generateName: workflow-template-hook- +spec: + workflowTemplateRef: + name: hook +`). + When(). + CreateWorkflowTemplates(). + SubmitWorkflow(). + WaitForWorkflow(fixtures.ToBeSucceeded). + Then(). + ExpectWorkflow(func(t *testing.T, metadata *v1.ObjectMeta, status *v1alpha1.WorkflowStatus) { + assert.Equal(t, status.Phase, v1alpha1.WorkflowSucceeded) + }). + ExpectWorkflowNode(func(status v1alpha1.NodeStatus) bool { + return strings.Contains(status.Name, "hooks.running") + }, func(t *testing.T, status *v1alpha1.NodeStatus, pod *apiv1.Pod) { + assert.Equal(t, v1alpha1.NodeSucceeded, status.Phase) + }). + ExpectWorkflowNode(func(status v1alpha1.NodeStatus) bool { + return strings.Contains(status.Name, "hooks.succeed") + }, func(t *testing.T, status *v1alpha1.NodeStatus, pod *apiv1.Pod) { + assert.Equal(t, v1alpha1.NodeSucceeded, status.Phase) + }) +} + func (s *WorkflowTemplateSuite) TestWorkflowTemplateInvalidEntryPoint() { s.Given(). WorkflowTemplate("@testdata/workflow-template-invalid-entrypoint.yaml"). diff --git a/workflow/util/merge.go b/workflow/util/merge.go index 7db69763f347..0789a104012f 100644 --- a/workflow/util/merge.go +++ b/workflow/util/merge.go @@ -39,6 +39,9 @@ func MergeTo(patch, target *wfv1.Workflow) error { return err } + if len(patchHooks) != 0 && target.Spec.Hooks == nil { + target.Spec.Hooks = make(wfv1.LifecycleHooks) + } for name, hook := range patchHooks { // If the patch hook doesn't exist in target if _, ok := target.Spec.Hooks[name]; !ok { diff --git a/workflow/util/merge_test.go b/workflow/util/merge_test.go index c9c83d7edc20..738047a21b35 100644 --- a/workflow/util/merge_test.go +++ b/workflow/util/merge_test.go @@ -272,6 +272,14 @@ func TestJoinWorkflowMetaData(t *testing.T) { }) } +var baseNilHookWF = ` +apiVersion: argoproj.io/v1alpha1 +kind: Workflow +metadata: + generateName: workflow-template-hello-world- +spec: +` + var baseHookWF = ` apiVersion: argoproj.io/v1alpha1 kind: Workflow @@ -284,6 +292,12 @@ spec: expression: workflow.status == "Pending" ` +var patchNilHookWF = ` +apiVersion: argoproj.io/v1alpha1 +kind: Workflow +spec: +` + var patchHookWF = ` apiVersion: argoproj.io/v1alpha1 kind: Workflow @@ -297,14 +311,36 @@ spec: expression: workflow.status == "Pending" ` -// Ensure hook bar ends up in result, but foo is unchanged func TestMergeHooks(t *testing.T) { - patchHookWf := wfv1.MustUnmarshalWorkflow(patchHookWF) - targetHookWf := wfv1.MustUnmarshalWorkflow(baseHookWF) + t.Run("NilBaseAndNilPatch", func(t *testing.T) { + patchHookWf := wfv1.MustUnmarshalWorkflow(patchNilHookWF) + targetHookWf := wfv1.MustUnmarshalWorkflow(baseNilHookWF) - err := MergeTo(patchHookWf, targetHookWf) - assert.NoError(t, err) - assert.Equal(t, 2, len(targetHookWf.Spec.Hooks)) - assert.Equal(t, "a", targetHookWf.Spec.Hooks[`foo`].Template) - assert.Equal(t, "b", targetHookWf.Spec.Hooks[`bar`].Template) + err := MergeTo(patchHookWf, targetHookWf) + assert.NoError(t, err) + assert.Nil(t, targetHookWf.Spec.Hooks) + }) + + t.Run("NilBaseAndNotNilPatch", func(t *testing.T) { + patchHookWf := wfv1.MustUnmarshalWorkflow(patchHookWF) + targetHookWf := wfv1.MustUnmarshalWorkflow(baseNilHookWF) + + err := MergeTo(patchHookWf, targetHookWf) + assert.NoError(t, err) + assert.Equal(t, 2, len(targetHookWf.Spec.Hooks)) + assert.Equal(t, "c", targetHookWf.Spec.Hooks[`foo`].Template) + assert.Equal(t, "b", targetHookWf.Spec.Hooks[`bar`].Template) + }) + + // Ensure hook bar ends up in result, but foo is unchanged + t.Run("NotNilBaseAndPatch", func(t *testing.T) { + patchHookWf := wfv1.MustUnmarshalWorkflow(patchHookWF) + targetHookWf := wfv1.MustUnmarshalWorkflow(baseHookWF) + + err := MergeTo(patchHookWf, targetHookWf) + assert.NoError(t, err) + assert.Equal(t, 2, len(targetHookWf.Spec.Hooks)) + assert.Equal(t, "a", targetHookWf.Spec.Hooks[`foo`].Template) + assert.Equal(t, "b", targetHookWf.Spec.Hooks[`bar`].Template) + }) }