Skip to content

Wire enableReplication into cli #881

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
Open
michaely520 wants to merge 6 commits into
base: next-server
Choose a base branch
from
Open

Wire enableReplication into cli #881

michaely520 wants to merge 6 commits into from

Conversation

@michaely520
Copy link

@michaely520 michaely520 commented Dec 3, 2025 •
edited
Loading

What was changed

Wiring in new enableReplication flag into cluster upsert cli.

Deleting sql pinning test as we no longer need this, see historical context:

#784
temporalio/temporal#7333

then unpinning in OSS temporalio/temporal#8489

Why?

Allows for users to leverage the new field

Checklist

  1. Closes

  2. How was this tested:

built locally and tested in conjunction with temporalio/temporal#8744, used the cli manually to make a replication based test pass

  1. Any docs updates needed?

@michaely520 michaely520 requested review from a team as code owners December 3, 2025 16:45
@CLAassistant
Copy link

CLAassistant commented Dec 3, 2025 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLAassistant commented Dec 3, 2025

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

This was referenced Dec 3, 2025
Merged
Merged
cretz
cretz reviewed Dec 3, 2025
View reviewed changes
go.mod Outdated
go.temporal.io/sdk v1.37.0
go.temporal.io/sdk/contrib/envconfig v0.1.0
go.temporal.io/server v1.29.1
go.temporal.io/server v1.30.0-147.0
Copy link
Member

@cretz cretz Dec 3, 2025

Choose a reason for hiding this comment

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

This PR needs to target next-server branch IMO (unsure if has had main merged into it recently or if it needs to)

michaely520 added a commit to temporalio/api that referenced this pull request Dec 3, 2025
@michaely520
Enable reading replicationEnabled (#678)
de2a88a 
<!-- Describe what has changed in this PR -->
**What changed?**
Enable reading of new field added in AddOrUpdateRemoteClusterResponse as
these apparently aren't sharing/reusing a common proto for
reading/writing

<!-- Tell your future self why have you made these changes -->
**Why?**
Enabling reads from cli

<!-- Are there any breaking changes on binary or code level? -->
**Breaking changes**
na

<!-- If this breaks the Server, please provide the Server PR to merge
right after this PR was merged. -->
**Server PR**
temporalio/temporal#8744

cli PR - temporalio/cli#881
temporal-cicd bot pushed a commit to temporalio/api-go that referenced this pull request Dec 3, 2025
@michaely520
Enable reading replicationEnabled (#678)
09837f6 
<!-- Describe what has changed in this PR -->
**What changed?**
Enable reading of new field added in AddOrUpdateRemoteClusterResponse as
these apparently aren't sharing/reusing a common proto for
reading/writing

<!-- Tell your future self why have you made these changes -->
**Why?**
Enabling reads from cli

<!-- Are there any breaking changes on binary or code level? -->
**Breaking changes**
na

<!-- If this breaks the Server, please provide the Server PR to merge
right after this PR was merged. -->
**Server PR**
temporalio/temporal#8744

cli PR - temporalio/cli#881
@michaely520 michaely520 changed the base branch from main to next-server December 9, 2025 16:16
@semgrep-managed-scans
Copy link

semgrep-managed-scans bot commented Dec 9, 2025

Semgrep found 3 missing-explicit-permissions findings:

No explicit GITHUB_TOKEN permissions found at the workflow or job level. Add a permissions: block at the workflow root (applies to all jobs) or per job with least privilege (e.g., contents: read and only specific writes like pull-requests: write if needed).

cretz
cretz reviewed Dec 9, 2025
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.

LGTM since it targets next-server, but will let someone on the CLI team provide approval

michaely520 added a commit to temporalio/temporal that referenced this pull request Dec 12, 2025
@michaely520
Wire in enableReplication into operator api (#8744)
f6ffb44 
## What changed?
Wiring in #8658 but for
operator api

## Why?
Compatibility with CLI

## How did you test it?
- [x] built
- [x] run locally and tested manually
- [ ] covered by existing tests
- [ ] added new unit test(s)
- [ ] added new functional test(s)

in our functional test I commented out 
```
	// Enable replication active -> standby
	_, err = activeCluster.AdminClient().AddOrUpdateRemoteCluster(
		ctx,
		&adminservice.AddOrUpdateRemoteClusterRequest{
			FrontendAddress:               standbyCluster.Host().RemoteFrontendGRPCAddress(),
			FrontendHttpAddress:           standbyCluster.Host().FrontendHTTPAddress(),
			EnableRemoteClusterConnection: true,
			EnableReplication:             true, // NOW enable replication
		})
	s.Require().NoError(err)

	// Enable replication standby -> active
	_, err = standbyCluster.AdminClient().AddOrUpdateRemoteCluster(
		ctx,
		&adminservice.AddOrUpdateRemoteClusterRequest{
			FrontendAddress:               activeCluster.Host().RemoteFrontendGRPCAddress(),
			FrontendHttpAddress:           activeCluster.Host().FrontendHTTPAddress(),
			EnableRemoteClusterConnection: true,
			EnableReplication:             true, // NOW enable replication
		})
	s.Require().NoError(err)
```

and used a locally built cli temporalio/cli#881
to enable, test passed
chaptersix pushed a commit to chaptersix/temporal that referenced this pull request Dec 15, 2025
@michaely520 @chaptersix
Wire in enableReplication into operator api (temporalio#8744)
e75062a 
## What changed?
Wiring in temporalio#8658 but for
operator api

## Why?
Compatibility with CLI

## How did you test it?
- [x] built
- [x] run locally and tested manually
- [ ] covered by existing tests
- [ ] added new unit test(s)
- [ ] added new functional test(s)

in our functional test I commented out 
```
	// Enable replication active -> standby
	_, err = activeCluster.AdminClient().AddOrUpdateRemoteCluster(
		ctx,
		&adminservice.AddOrUpdateRemoteClusterRequest{
			FrontendAddress:               standbyCluster.Host().RemoteFrontendGRPCAddress(),
			FrontendHttpAddress:           standbyCluster.Host().FrontendHTTPAddress(),
			EnableRemoteClusterConnection: true,
			EnableReplication:             true, // NOW enable replication
		})
	s.Require().NoError(err)

	// Enable replication standby -> active
	_, err = standbyCluster.AdminClient().AddOrUpdateRemoteCluster(
		ctx,
		&adminservice.AddOrUpdateRemoteClusterRequest{
			FrontendAddress:               activeCluster.Host().RemoteFrontendGRPCAddress(),
			FrontendHttpAddress:           activeCluster.Host().FrontendHTTPAddress(),
			EnableRemoteClusterConnection: true,
			EnableReplication:             true, // NOW enable replication
		})
	s.Require().NoError(err)
```

and used a locally built cli temporalio/cli#881
to enable, test passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cretz cretz cretz left review comments

@xwduan xwduan xwduan 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.

5 participants

@michaely520 @CLAassistant @cretz @xwduan