Skip to content

Remove infinite version existence check#98

Merged
carlydf merged 1 commit intotemporalio:mainfrom
carlydf:remove-version-check
Aug 1, 2025
Merged

Remove infinite version existence check#98
carlydf merged 1 commit intotemporalio:mainfrom
carlydf:remove-version-check

Conversation

@carlydf
Copy link
Copy Markdown
Collaborator

@carlydf carlydf commented Jul 29, 2025

What was changed

What it says in the title.
Replaced with Version Status == NotRegistered condition in planning phase.
Also reorganized test helper functions, ensured that Deployments created by test helpers can be found by the controller, and added a failed and successful Gate workflow test.

Why?

Because checking for version existence in execplan.go breaks the abstraction of only reading server status in gen status. And we have the information to not do another describe, so we should use it.

Changes to test files are so that helper functions and test functions are separated and easier to read and work on.

Checklist

  1. Closes Remove infinite loop from Version Registration check #57

  2. How was this tested:
    New tests specific to Gate workflows ensure that we are not starting those workflows on a non-existent version, and old tests also show that SetCurrent and SetRamping calls are not erroring due to nonexistent version.

  3. Any docs updates needed?
    no

@carlydf carlydf requested review from a team and jlegrone as code owners July 29, 2025 00:07
@jlegrone jlegrone requested a review from Copilot July 30, 2025 18:19
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR removes the infinite version existence check that was performed before registering versions with Temporal, replacing it with a condition to check if the version status is NotRegistered in the planning phase. This improves abstraction by avoiding redundant server calls when status information is already available.

Key changes:

  • Replace version existence checks with NotRegistered status conditions in planning phase
  • Reorganize test helper functions into separate files for better maintainability
  • Add new Gate workflow tests for both successful and failed scenarios

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
internal/controller/util.go Remove awaitVersionRegistration and related functions
internal/controller/execplan.go Remove version existence checks before registering versions
internal/planner/planner.go Add NotRegistered status checks to prevent actions on unregistered versions
internal/k8s/deployments.go Add NewDeploymentWithControllerRef helper function
internal/controller/genplan.go Simplify deployment creation using new helper
internal/testhelpers/workers.go Move worker functions to testhelpers package and add Gate workflow types
internal/tests/internal/deployment_controller.go Extract deployment controller functions from env_helpers
internal/tests/internal/env_helpers.go Remove deployment functions, add testEnv struct
internal/tests/internal/integration_test.go Add Gate workflow tests and reorganize test structure
internal/tests/internal/validation_helpers.go Add nil check and fix error message terminology
internal/testhelpers/test_builder.go Add Gate configuration and wait time support

Comment thread internal/testhelpers/workers.go
Comment thread internal/testhelpers/workers.go
Comment thread internal/testhelpers/workers.go
Comment thread internal/testhelpers/workers.go
Comment thread internal/tests/internal/validation_helpers.go
@carlydf carlydf merged commit 5b2ad52 into temporalio:main Aug 1, 2025
11 checks passed
@carlydf carlydf deleted the remove-version-check branch August 1, 2025 20:30
shashwatsuri pushed a commit to shashwatsuri/temporal-worker-controller that referenced this pull request Apr 28, 2026
<!--- Note to EXTERNAL Contributors -->
<!-- Thanks for opening a PR! 
If it is a significant code change, please **make sure there is an open
issue** for this.
We work best with you when we have accepted the idea first before you
code. -->

<!--- For ALL Contributors 👇 -->

## What was changed
What it says in the title.
Replaced with Version Status == NotRegistered condition in planning
phase.
Also reorganized test helper functions, ensured that Deployments created
by test helpers can be found by the controller, and added a failed and
successful Gate workflow test.

## Why?
Because checking for version existence in execplan.go breaks the
abstraction of only reading server status in gen status. And we have the
information to not do another describe, so we should use it.

Changes to test files are so that helper functions and test functions
are separated and easier to read and work on.

## Checklist
<!--- add/delete as needed --->

1. Closes temporalio#57 

2. How was this tested:
New tests specific to Gate workflows ensure that we are not starting
those workflows on a non-existent version, and old tests also show that
SetCurrent and SetRamping calls are not erroring due to nonexistent
version.

3. Any docs updates needed?
no
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.

Remove infinite loop from Version Registration check

3 participants