Skip to content

chore: Go 1.24, and mockery, x/tools, sqlite to support it#7244

Closed
Groxx wants to merge 9 commits into
cadence-workflow:masterfrom
Groxx:mockery-v3
Closed

chore: Go 1.24, and mockery, x/tools, sqlite to support it#7244
Groxx wants to merge 9 commits into
cadence-workflow:masterfrom
Groxx:mockery-v3

Conversation

@Groxx

@Groxx Groxx commented Sep 9, 2025

Copy link
Copy Markdown
Member

After much yamling of files, Go 1.24 support is ready!
This is the non-WIP version of #7111 which got kicked off due to #7093 (but it does not contain those changes, this is just roughly the minimum needed for 1.24).

Library changes

This migrates a relative few mocks to github.com/vektra/mockery/v3, because v2 doesn't support Go 1.24+.
The good news is that it's MUCH faster now, and more flexible, though only for a few types.

If vektra/mockery#1030 gets accepted, we could do the same for our go.uber.org/mock mocks, but currently the license is questionable in the fork (for CNCF purposes) and the changes are non-trivial. So getting mockery to accept it (which might involve go.uber.org/mock accepting it) is likely the best route, for a variety of reasons.

go.uber.org/mock's upgrade seems mostly harmless and I was hoping it'd solve some of the test failures (it did not), so I've just left it in. Unfortunately it actually makes mock-codegen worse, ~1 arg per mock function on some types is renamed from something useful to arg0 or arg4 (position varies). I have no idea why that happens, but it doesn't seem too critical to maintain, and it's stable so it's not a codegen concern.

github.com/ncruces/go-sqlite3 has a minimal upgrade to support go 1.24, as we were getting segfaults and wasm panics without the upgrade. We could upgrade it to latest, but I'm holding off on that for the moment because it might be harder to achieve in our monorepo :| (apologies to OSS folks)

signalDelayCount++
if signalDelayCount > params.MaxSignalDelayCount {
return fmt.Errorf(fmt.Sprintf("received %v signals are longer than %v seconds.", params.MaxSignalDelayCount, params.MaxSignalDelayInSeconds))
return fmt.Errorf("received %v signals are longer than %v seconds", params.MaxSignalDelayCount, params.MaxSignalDelayInSeconds)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

new vet check, required to compile tests.

metricsScope := &mocks.Scope{}
metricsClient.
On("Scope", metrics.MessagingClientPublishScope, metrics.TopicTag("test-topic-1")).
On("Scope", metrics.MessagingClientPublishScope, []metrics.Tag{metrics.TopicTag("test-topic-1")}).

@Groxx Groxx Sep 9, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

pickier about types, it seems.
this doesn't change between On and EXPECT().Method.

given that the codegen knows this is a varargs, I think this is probably a codegen/mock-library gap that should be fixed. but we have few uses so it's really not a blocker for us.

})
if err != nil {
return false, c.failed("failed to get concrete execution record", err.Error())
return false, c.failed("failed to get concrete execution record", "%v", err)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

also forced by the new vet check. it detects calls through funcs! neat.

&& rm -rf /var/lib/apt/lists/*

RUN pip install cqlsh
RUN pip install cqlsh --break-system-packages

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

so glad python started forcing this

Comment on lines -658 to -659
seed := int64(1)
rand.Seed(seed)

@Groxx Groxx Sep 9, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

https://tip.golang.org/doc/go1.24#mathrandpkgmathrand

Calls to the deprecated top-level Seed function no longer have any effect. To restore the old behavior use GODEBUG setting randseednop=0. For more background see proposal #67273.

we have a few other seed calls, but they don't seem to be load-bearing like the ones in this test were.
so I just worked around this with a custom (sadly verbose) matcher.

Comment thread Dockerfile

# Build Cadence binaries
FROM golang:1.23.4-alpine3.21 AS builder
FROM golang:1.24.7-alpine3.21 AS builder

@Groxx Groxx Sep 9, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I tried a bit to convert our github-action to alpine too, but hit too many barriers and gave up fairly quickly :|

probably doesn't matter if the actions/setup-go ones are debian/ubuntu? though.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

yeah, as long as the images are cached, debian's honestly usually a little easier to work with

Comment thread Makefile

# lints that go modules are as expected, e.g. parent does not import submodule.
# tool builds that need to be in sync with the parent are partially checked through go_mod_build_tool, but should probably be checked here too
$(BUILD)/gomod-lint: go.mod internal/tools/go.mod common/archiver/gcloud/go.mod | $(BUILD)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

was missing a couple

Comment thread Makefile
$Q # tidy in dependency order
$Q # tidy needs to run in dependency order:
$Q # everything depends on the main module, server depends on everything else so it should be last.
$Q # but we can just tidy everything 2x, which should be stable as our dependency tree is only 2 deep.

@Groxx Groxx Sep 9, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

tried to come up with a different approach, but nothing seemed more reliable than "just run it twice lol". and we can easily increase it if we ever have a 3-deep chain, still without caring about their dependency orders - reruns are generally less than a second so it hardly matters.


// GetRemoteAdminClient mocks base method.
func (m *MockResource) GetRemoteAdminClient(cluster string) (admin.Client, error) {
func (m *MockResource) GetRemoteAdminClient(arg0 string) (admin.Client, error) {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

this is one of those "worse codegen from gomock" cases


// UpdateTimerProcessingQueueStates mocks base method.
func (m *MockContext) UpdateTimerProcessingQueueStates(cluster string, states []*types.ProcessingQueueState) error {
func (m *MockContext) UpdateTimerProcessingQueueStates(arg0 string, states []*types.ProcessingQueueState) error {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

see, this one changes the first, not the second, while others change the second but not the first.

if it wasn't stable, I'd be concerned, but it re-gens the same every time so...


// GetQueueClusterAckLevel mocks base method.
func (m *MockContext) GetQueueClusterAckLevel(category persistence.HistoryTaskCategory, cluster string) persistence.HistoryTaskKey {
func (m *MockContext) GetQueueClusterAckLevel(category persistence.HistoryTaskCategory, arg1 string) persistence.HistoryTaskKey {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

changes the second, not the first

Signed-off-by: Steven L <imgroxx@gmail.com>
Comment thread .mockery.yml
@@ -0,0 +1,85 @@
all: false

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nice, is this intended to be comprehensive or it's just some problematic ones?

@Groxx Groxx Sep 24, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

"all" is for whether it'll generate mocks of every interface it discovers, or if it's an explicit list (below). e.g. you could use it for "all" in a subdir, if it always contained interfaces you wanted to mock (like an "interfaces" dir).

I don't think we'll ever get even close to wanting that tho :)

Comment thread Makefile
$Q cd common/archiver/gcloud; go mod tidy || (echo "failed to tidy gcloud plugin, try manually copying go.mod contents into common/archiver/gcloud/go.mod and rerunning" >&2; exit 1)
$Q cd cmd/server; go mod tidy || (echo "failed to tidy main server module, try manually copying go.mod and common/archiver/gcloud/go.mod contents into cmd/server/go.mod and rerunning" >&2; exit 1)
$Q $(foreach sub,$(SUBMODULE_PATHS), \
$Q cd $(sub); go mod tidy || $$(>&2 echo "failed to tidy $(sub) plugin, try manually copying go.mod contents into $(sub)/go.mod and rerunning by hand"; exit 1)$(NEWLINE) \

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

no change requested, but this is getting into probably-should-be-a-script territory in future.

// to the source files. Usage as follows:
//
// ./cmd/tools/copyright/licensegen.go
func main() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

what's the story with this? I wasn't following along, do we not need it?

@Groxx Groxx Sep 24, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

mostly just leftovers from #6882 which isn't very descriptive. but it has been completely disabled since that point.

AFAICT legal finally got back to us and said

yea you just need a LICENSE file, stuff on each file never had any legal impact
(aside from cases where you need to carry along attribution/etc).

so it was just legal-cargo-culting all along.

@davidporter-id-au davidporter-id-au left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm, just wondering what the deal is with the license gen helper, we don't need it?

@davidporter-id-au

Copy link
Copy Markdown
Member

also there's some check stuff failing, but unsure what the deal was there

Comment thread common/blobstore/client_mock.go
Comment thread common/archiver/provider/provider_mock.go
required changing a couple mocks and regenerating a bit, but nothing major

Signed-off-by: Steven L <imgroxx@gmail.com>
Signed-off-by: Steven L <imgroxx@gmail.com>
Signed-off-by: Steven L <imgroxx@gmail.com>
Signed-off-by: Steven L <imgroxx@gmail.com>
Signed-off-by: Steven L <imgroxx@gmail.com>
@Groxx Groxx changed the title Upgrade Go to 1.24, and upgrade mockery, x/tools Upgrade Go to 1.24, and upgrade mockery, x/tools, sqlite Sep 25, 2025
@Groxx Groxx changed the title Upgrade Go to 1.24, and upgrade mockery, x/tools, sqlite upgrade: Go 1.24, and mockery, x/tools, sqlite to support it Sep 30, 2025
@Groxx Groxx changed the title upgrade: Go 1.24, and mockery, x/tools, sqlite to support it chore: Go 1.24, and mockery, x/tools, sqlite to support it Sep 30, 2025
@c-warren c-warren self-assigned this May 12, 2026
c-warren added a commit that referenced this pull request Jun 3, 2026
<!-- If you are new to contributing or want a refresher, please read
./pull_request_guidance.md -->
**What changed?**

This PR is a continuation of
#7244 from @Groxx,
adding the mockery v3 changes on top of a previous upgrade to go1.24.

This also removes the license checker from our PR requirements, as
LICENSE and NOTICE files are the recommended way to do it.

**Why?**

See #7244 for details. 

**How did you test it?**

The ol' local tests. 

**Potential risks**

This is mostly mock changes and internal tooling rather than any
functional changes, so it should be pretty low risk.

**Release notes**

N/A

**Documentation Changes**

N/A

---------

Signed-off-by: Steven L <imgroxx@gmail.com>
Co-authored-by: Steven L <imgroxx@gmail.com>
Co-authored-by: Vishwa Patil <vpatil16@ext.uber.com>
@c-warren

c-warren commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Thank you @Groxx :)

Finished off here: #8161

@c-warren c-warren closed this Jun 3, 2026
YaweiZhang-930 pushed a commit to YaweiZhang-930/cadence that referenced this pull request Jun 3, 2026
<!-- If you are new to contributing or want a refresher, please read
./pull_request_guidance.md -->
**What changed?**

This PR is a continuation of
cadence-workflow#7244 from @Groxx,
adding the mockery v3 changes on top of a previous upgrade to go1.24.

This also removes the license checker from our PR requirements, as
LICENSE and NOTICE files are the recommended way to do it.

**Why?**

See cadence-workflow#7244 for details.

**How did you test it?**

The ol' local tests.

**Potential risks**

This is mostly mock changes and internal tooling rather than any
functional changes, so it should be pretty low risk.

**Release notes**

N/A

**Documentation Changes**

N/A

---------

Signed-off-by: Steven L <imgroxx@gmail.com>
Co-authored-by: Steven L <imgroxx@gmail.com>
Co-authored-by: Vishwa Patil <vpatil16@ext.uber.com>
Signed-off-by: YaweiZhang-930 <yawei930@gmail.com>
YaweiZhang-930 pushed a commit to YaweiZhang-930/cadence that referenced this pull request Jun 3, 2026
<!-- If you are new to contributing or want a refresher, please read
./pull_request_guidance.md -->
**What changed?**

This PR is a continuation of
cadence-workflow#7244 from @Groxx,
adding the mockery v3 changes on top of a previous upgrade to go1.24.

This also removes the license checker from our PR requirements, as
LICENSE and NOTICE files are the recommended way to do it.

**Why?**

See cadence-workflow#7244 for details.

**How did you test it?**

The ol' local tests.

**Potential risks**

This is mostly mock changes and internal tooling rather than any
functional changes, so it should be pretty low risk.

**Release notes**

N/A

**Documentation Changes**

N/A

---------

Signed-off-by: Steven L <imgroxx@gmail.com>
Co-authored-by: Steven L <imgroxx@gmail.com>
Co-authored-by: Vishwa Patil <vpatil16@ext.uber.com>
Signed-off-by: YaweiZhang-930 <yawei930@gmail.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.

4 participants