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

workflowcheck built on 1.23 does not find any findings #1627

Closed
aaomidi opened this issue Sep 9, 2024 · 4 comments
Closed

workflowcheck built on 1.23 does not find any findings #1627

aaomidi opened this issue Sep 9, 2024 · 4 comments

Comments

@aaomidi
Copy link

aaomidi commented Sep 9, 2024

We use this method for managing tools: https://marcofranssen.nl/manage-go-tools-via-go-modules

We've updated that go.mod to 1.23.0, and now when we run workflowcheck it has no findings. This is pretty easy to replicate, you can update the go.mod file in https://github.com/temporalio/sdk-go/blob/master/contrib/tools/workflowcheck/go.mod and see that workflowcheck isn't able to do proper analysis.

This code works properly up until 1.22.7.

@aaomidi
Copy link
Author

aaomidi commented Sep 10, 2024

I think this also shows a potential need for more test cases for the workflowcheck code, as updating the go sdk does not recognize that the analyzer itself actually stops working.

@Quinn-With-Two-Ns
Copy link
Contributor

I tried updating the workflowcheck to 1.23.0 and see no test failure. Can you provided an example of something that was detected as none deterministic before but is no longer being properly detected?

Note the workflowcheck has no dependency on the Go SDK so an upgrade to the SDK would not impact workflowcheck

@aaomidi
Copy link
Author

aaomidi commented Sep 13, 2024

package workflows

import (
	"time"

	"go.temporal.io/sdk/workflow"
)

type SomeInterestingWorkInput struct{}

type SomeInterestingWorkOutput struct {
	HappenedAt int64
}

func SomeInterestingWork(ctx workflow.Context, input SomeInterestingWorkInput) (SomeInterestingWorkOutput, error) {
	// Do some interesting work here
	return SomeInterestingWorkOutput{
		HappenedAt: time.Now().Unix(),
	}, nil
}

If I install workflowcheck using go install go.temporal.io/sdk/contrib/tools/workflowcheck@latest, which I believe respects the go.mod file that workflowcheck has, it finds the time.Now() issue. If I clone the repo myself, change the go version, and do a go install . on it, then it stops being able to find them.

@sync-by-unito sync-by-unito bot closed this as completed Sep 24, 2024
@aaomidi
Copy link
Author

aaomidi commented Sep 24, 2024

Thank you for finding this. Seems like quite a breaking change from Go!

Link to the PR: #1642

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants