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: Support memoization on plugin node. Fixes #8553 #8554

Merged
merged 8 commits into from
May 2, 2022

Conversation

wujayway
Copy link
Contributor

Signed-off-by: wujayway [email protected]

Fixes #8553

@wujayway wujayway changed the title Fix#8553: support memoization on plugin node. fix: Support memoization on plugin node. Fixes#8553 Apr 30, 2022
@@ -134,6 +137,13 @@ func (woc *wfOperationCtx) reconcileTaskSet(ctx context.Context) error {
node.FinishedAt = metav1.Now()

woc.wf.Status.Nodes[nodeID] = node
if node.MemoizationStatus != nil && node.Succeeded() {
Copy link
Contributor

Choose a reason for hiding this comment

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

test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

test?

done

@alexec alexec changed the title fix: Support memoization on plugin node. Fixes#8553 fix: Support memoization on plugin node. Fixes #8553 Apr 30, 2022
@wujayway
Copy link
Contributor Author

wujayway commented May 1, 2022

Hi @alexec ,
ut not work after merge #8557.
It looks like the ut assume the order of node. But node is a map which made ut unstable, pass in random


func TestReconcileTaskSetWithMemoization(t *testing.T) {
wf := wfv1.MustUnmarshalWorkflow(`apiVersion: argoproj.io/v1alpha1
kind: Workflow
Copy link
Contributor

@alexec alexec May 1, 2022

Choose a reason for hiding this comment

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

This example's length makes it too hard to understand what is the example is relevant to the test, and what is just copied and pasted. Can you please simplify.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

wujayway added 2 commits May 2, 2022 00:54
Signed-off-by: wujayway <[email protected]>
Signed-off-by: wujayway <[email protected]>
@@ -829,24 +829,40 @@ func TestWorkflow_SearchArtifacts(t *testing.T) {
queriedArtifactSearchResults := wf.SearchArtifacts(query)
assert.NotNil(t, queriedArtifactSearchResults)
assert.Len(t, queriedArtifactSearchResults, 3)
assert.Equal(t, "artifact-foo", queriedArtifactSearchResults[0].Artifact.Name)
Copy link
Contributor

Choose a reason for hiding this comment

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

?

Copy link
Contributor

Choose a reason for hiding this comment

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

what is this please?

Copy link
Contributor

Choose a reason for hiding this comment

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

@rohankmr414 maybe this is the fix?

Copy link
Member

Choose a reason for hiding this comment

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

yeah looks like it

@alexec alexec enabled auto-merge (squash) May 2, 2022 02:02
@alexec alexec merged commit dc8fef3 into argoproj:master May 2, 2022
@alexec alexec mentioned this pull request May 3, 2022
alexec pushed a commit that referenced this pull request May 3, 2022
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.

Memoization not work on plugin node.
3 participants