-
Notifications
You must be signed in to change notification settings - Fork 8
Remove Ramping version from status #80
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
Remove Ramping version from status #80
Conversation
Tidy up some tests, removing state that isn't relevant.
There was a problem hiding this 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 deprecated RampingVersion
field from the status API and updates the planner and controller layers to drive all version transitions using the TargetVersion
, CurrentVersion
, and a new TemporalWorkerState
. Key changes:
- Eliminate the
RampingVersion
structure from CRDs, deepcopy code, and status mapping. - Change
GeneratePlan
and its helper functions to take explicitstatus
,spec
, andtemporalState
parameters rather than embedding much of that inConfig
. - Update all tests and controller
generateStatus
/generatePlan
calls to the new signature and behavior.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
internal/planner/planner.go | Changed planner helpers to use status and spec instead of Config.* and removed ramping version logic |
internal/planner/planner_test.go | Updated every test to pass the new status , spec , and state into GeneratePlan , plus removed references to ramping |
internal/controller/state_mapper.go | Removed mapping of RampingVersion and updated mapping of CurrentVersion and TargetVersion |
internal/controller/genstatus.go | Pulled in temporalState earlier and passed it through to status generation |
internal/controller/genplan.go | Adapted controller’s plan-generation call to the new planner signature |
helm/.../temporal.io_temporalworkerdeployments.yaml | Removed the rampingVersion properties from the CRD schema |
api/v1alpha1/zz_generated.deepcopy.go | Removed deep-copy logic for RampingVersion |
api/v1alpha1/worker_types.go | Deleted the RampingVersion field on TemporalWorkerDeploymentStatus |
Comments suppressed due to low confidence (2)
internal/controller/state_mapper.go:36
- This unconditionally maps an empty
CurrentVersionID
to a non-nil status object. Restore the originalif currentVersionID != ""
guard so thatCurrentVersion
remains nil when there is no active version.
status.CurrentVersion = m.mapCurrentWorkerDeploymentVersion(currentVersionID)
internal/controller/state_mapper.go:101
- The function no longer returns nil for an empty
versionID
, which can cause a phantom target version in status. Reintroduce theif versionID == "" { return nil }
check to preserve the original semantics.
func (m *stateMapper) mapTargetWorkerDeploymentVersion(versionID string) *v1alpha1.TargetWorkerDeploymentVersion {
Co-pilot's state_mapper comment about |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, just see last comment about not using a pointer value for TargetVersion
-- no worries if there are other reasons to prefer a pointer though
This is clearer than leaving it as a pointer and validating.
What was changed
Removed Ramping version from the status.
Also adjusted code around the assumed presence of CurrentVersion and TargetVersion which was inverted in comments and code.
Why?
Seeing the Ramping Version in the status may confuse users.
Checklist
Closes
How was this tested: