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.

Sign up for GitHub

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

Jump to bottom

Nexus #1555

Merged
merged 13 commits into from
Jul 22, 2024
Merged

Nexus #1555

merged 13 commits into from
Jul 22, 2024

Conversation

bergundy
Copy link
Member

@bergundy bergundy commented Jul 19, 2024 •
edited
Loading

What was changed

  • Added the temporalnexus package and implemented the handler side for Nexus, including registering and dispatching Nexus Operations.
  • Added the ability to execute Nexus Operations from a workflow.
  • Added basic support for running Nexus Operations in the test environment.
  • Added memoizing to worker.Start() to return consistent errors to callers and avoid rerunning the function unnecessarily.
  • Updated the integration test's dev server to run CLI 0.14.0-nexus.0 which includes server 1.25.0-rc.0.

See the proposal for more information.

Most of this code has been reviewed already in #1466, #1473, and #1475, which are all squashed in the first commit.

bergundy and others added 2 commits July 18, 2024 17:21
@bergundy
Nexus (#1466)
648d46d 
* Nexus Handler
* Execute nexus operation from a workflow
* Add test environment support for Nexus Operations
@bergundy
Update API dep and Nexus tests
1598574
@bergundy bergundy requested a review from a team as a code owner July 19, 2024 18:43
cretz
cretz reviewed Jul 19, 2024
View reviewed changes
go.mod
@@ -1,32 +1,37 @@
module go.temporal.io/sdk

go 1.20
go 1.21
Copy link
Member

@cretz cretz Jul 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hrmm, do we want to bump minimum Go version? Not that I mind, just want to make sure it's a conscious decision and not done accidentally in this PR

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure why this happened, maybe because the nexus SDK depends on slog?

Copy link
Member

@cretz cretz Jul 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, or more importantly, Nexus Go SDK itself requires 1.21+ which infects all users of it. Technically that's the oldest non-EOL Go version anyways, @Quinn-With-Two-Ns - thoughts?

Also, another concern I now have is about transitive dependencies. Nexus Go SDK is now forcing github.com/gorilla/mux to at least v1.8.0 for every user of the Temporal Go SDK. I am not sure we want to force this dependency or our needed version of it on every user. How important is that dependency in the Nexus Go SDK?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm sure we can do without this dependency. I know Go's built-in router has advanced in recent versions.
Can we live with this for the immediate future and investigate next week?

Copy link
Member

@cretz cretz Jul 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry, didn't see the reply (was just after business hours Fri). I would rather not impose this dependency on users even temporarily.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah github.com/gorilla/mux is quite a rough dependency to start requiring for non nexus users.

Copy link
Contributor

Choose a reason for hiding this comment

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

Upgrading to go 1.21 is fine if it is needed

go.mod Outdated Show resolved Hide resolved
@bergundy bergundy force-pushed the nexus branch 7 times, most recently from d93b8ed to 12f4392 Compare July 19, 2024 19:18
bergundy
bergundy commented Jul 19, 2024
View reviewed changes
testsuite/devserver.go Outdated
@@ -199,6 +199,7 @@ func downloadIfNeeded(ctx context.Context, options *DevServerOptions, logger log
return "", fmt.Errorf("unsupported architecture %v", arch)
}
infoURL := fmt.Sprintf("https://temporal.download/cli/%v?platform=%v&arch=%v&sdk-name=sdk-go&sdk-version=%v", url.QueryEscape(version), platform, arch, internal.SDKVersion)
fmt.Println(infoURL)
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm removing this.

cretz
cretz approved these changes Jul 22, 2024
View reviewed changes
Copy link
Member

@cretz cretz left a comment

Choose a reason for hiding this comment

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

Would like CI to pass though, and need to make sure that we make clear in release notes that we now require Go 1.21+.

@bergundy bergundy enabled auto-merge (squash) July 22, 2024 21:00
@bergundy bergundy merged commit 50e993e into master Jul 22, 2024
13 of 14 checks passed
@bergundy bergundy deleted the nexus branch July 22, 2024 21:33
@dhiaayachi dhiaayachi mentioned this pull request Sep 18, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Quinn-With-Two-Ns Quinn-With-Two-Ns Quinn-With-Two-Ns approved these changes

@cretz cretz cretz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bergundy @cretz @Quinn-With-Two-Ns