[Breaking change] Follow k8s API conventions#136
Merged
Conversation
jlegrone
commented
Sep 4, 2025
jlegrone
added a commit
that referenced
this pull request
Sep 4, 2025
- Simplify shortName for TemporalConnection to just "tconn" - Remove print column annotations and replace with TODO comments linking to k8s conventions - Add TODO comments for unit test cases on reference types Per code review feedback on PR #136.
carlydf
reviewed
Sep 5, 2025
carlydf
reviewed
Sep 5, 2025
carlydf
reviewed
Sep 5, 2025
jlegrone
commented
Sep 5, 2025
jlegrone
commented
Sep 5, 2025
jlegrone
commented
Sep 5, 2025
jlegrone
commented
Sep 8, 2025
jlegrone
commented
Sep 9, 2025
jlegrone
force-pushed
the
jlegrone/k8s-api-conventions
branch
from
13880da to
8f3d89f
Compare
This change adds a conditions field to TemporalWorkerDeploymentStatus to conform with Kubernetes API conventions for status reporting. The conditions field provides a standardized way for controllers to communicate the current state of the resource. References: - K8s API Conventions: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#typical-status-properties
This change replaces the placeholder status implementation with meaningful fields including standard conditions, connection timing, and server version information. This provides proper observability into the connection state and conforms with Kubernetes API conventions. References: - K8s API Conventions: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#typical-status-properties
This change replaces TODO comments and placeholder text with proper godoc documentation to improve API usability and conform with Kubernetes documentation conventions. References: - K8s API Conventions: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#documentation
This change adds kubebuilder validation markers to ensure proper format validation for HostPort (host:port format) and MutualTLSSecret (Kubernetes resource name format) fields, improving API robustness. References: - K8s API Conventions: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#validation
Replace RampPercentage fields from float32 to int32 and add validation markers to comply with Kubernetes API conventions. Kubernetes conventions discourage floating-point values, especially in specs: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#types-kinds
Add +kubebuilder:validation:Minimum=0 to numeric count fields to comply with Kubernetes API validation conventions. Per conventions, numeric fields should be bound-checked: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#validation
Add omitempty to JSON tags for fields marked with +optional to ensure consistent optional field behavior. Per Kubernetes metadata conventions, optional fields should omit empty values: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#metadata
Add shortName and printcolumn annotations to improve kubectl CLI experience for TemporalConnection resources. Per Kubernetes metadata conventions for better CLI usability: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#metadata
Add validation patterns for TemporalConnection resource name and minimum length validation for TemporalNamespace field. Per Kubernetes validation conventions for proper field validation: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#validation
Add +kubebuilder:default markers for all fields with documented default values to comply with Kubernetes API defaulting conventions. Per Kubernetes conventions on proper default value specification: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#defaulting
Replace string-based references with corev1.LocalObjectReference for TemporalConnection and MutualTLSSecret fields to provide stronger typing and better validation. Per Kubernetes conventions for object references: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#object-references
Remove +optional marker from Selector field as it is functionally required for a Deployment-like resource, following the same pattern as Kubernetes Deployment. Per Kubernetes optional vs required field conventions: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#optional-vs-required
Revert to string-based references to maintain backward compatibility.
While LocalObjectReference is more typed, it would introduce a breaking
API change requiring users to change from "name" to {"name": "name"}.
Instead, keep string references but add proper validation patterns
for resource names.
Per Kubernetes API compatibility guidelines, avoid breaking changes
during incremental improvements.
Use LocalObjectReference with proper "Ref" suffix naming convention for object reference fields, following Kubernetes API conventions. Changes: - TemporalConnection -> TemporalConnectionRef - MutualTLSSecret -> MutualTLSSecretRef Per Kubernetes object reference conventions: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#object-references
Create specific reference types (TemporalConnectionReference, SecretReference) instead of using LocalObjectReference, following Kubernetes API conventions. LocalObjectReference godoc states: "New uses of this type are discouraged" and recommends creating "locally provided and used type that is well-focused on your reference." This provides: - Clear validation rules specific to each reference type - Better API documentation and help text - Consistent validation behavior Per Kubernetes object reference best practices: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#object-references
Add maximum validation to ensure int64 fields stay within the safe range for JSON serialization (-(2^53) < x < (2^53)) as required by Kubernetes primitive types guidelines. Per Kubernetes primitive types conventions for int64 fields: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#primitive-types
Replace TasksAddRate and TasksDispatchRate float32 fields with int64 fixed-point representation (TasksAddRateMillis, TasksDispatchRateMillis) to comply with Kubernetes primitive types guidelines. Kubernetes conventions state: "Avoid floating-point values as much as possible, and never use them in spec". Fixed-point representation provides precise, consistent serialization across languages. Rate values are expressed as tasks per second * 1000 for millisecond precision. Per Kubernetes primitive types guidelines: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#primitive-types
Update test files to use the renamed RampPercentage field instead of RampPercentageBasisPoints and adjust the percentage calculations since the field now stores direct percentages rather than basis points.
- Clean worker deployment names before using them as label values - Replace invalid characters (/) with valid ones (-) - Truncate to 63 character limit for Kubernetes labels Fixes https://github.com/temporalio/temporal-worker-controller/actions/runs/17591212096/job/49972137654
jlegrone
force-pushed
the
jlegrone/k8s-api-conventions
branch
from
878b911 to
157d3b3
Compare
- Replace nil logger with properly configured structured logger - Set log level to warn to reduce test noise - Eliminates 'No logger configured for temporal client' warning messages
- Change ProgressiveStep to treat input as direct percentage (5 = 5%) - Remove division by 100 that was causing rampPercentage: 0 validation errors - Fixes integration test failures due to k8s API validation requirements
carlydf
reviewed
Sep 9, 2025
- Update MakeTargetVersion to use direct percentage values instead of basis points - Remove division by 100 that was causing test expectation mismatches - Update WithTargetVersion parameter naming from rampPercentageBasisPoints to rampPercentage - Aligns test expectations with controller behavior after ProgressiveStep fix This fixes the remaining integration test failures: - progressive-rollout-yes-unversioned-pollers-expect-first-step - nth-progressive-rollout-expect-first-step - nth-progressive-rollout-with-success-gate
- Update VersionConfig struct to use RampPercentage instead of RampPercentageBasisPoints - Convert all percentage calculations from basis points to direct percentages (0-100) - Fix test cases to use percentage values instead of basis point values - Remove basis point conversion logic (* 100 and / 100 operations) - Update variable names and comments to reflect percentage usage
Changes: - Update handleProgressiveRollout to accept *float32 instead of *int32 for targetRampPercentage - Remove unnecessary type conversion from float32 to int32 in getVersionConfigDiff - Update getCurrentStepIndex function signature to match new type - Simplify test code by removing redundant type conversion This ensures type consistency throughout the progressive rollout logic while maintaining the same functionality.
jlegrone
commented
Sep 9, 2025
jlegrone
commented
Sep 9, 2025
<!--- 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 👇 -->
Stop these two warnings from happening on every reconcile loop
```
metadata.name: this is used in Pod names and hostnames, which can result in surprising behavior; a DNS label is recommended: [must not contain dots] {"controller": "temporalworkerdeployment", "controllerGroup": "temporal.io", "controllerKind": "TemporalWorkerDeployment", "TemporalWorkerDeployment": {"name":"all-at-once-rollout-2-replicas","namespace":"test-integration-20250903200429"}, "namespace": "test-integration-20250903200429", "name": "all-at-once-rollout-2-replicas", "reconcileID": "5cdf5048-693b-4f77-90ca-30d42444748e"}
```
```
{"level":"info","ts":1755712330.9777546,"msg":"unknown field \"spec.template.metadata.creationTimestamp\"","controller":"temporalworkerdeployment","controllerGroup":"temporal.io","controllerKind":"TemporalWorkerDeployment","TemporalWorkerDeployment":{"name":"helloworld","namespace":"staging"},"namespace":"staging","name":"helloworld","reconcileID":"0fe93d52-0049-437a-8e6e-1ebec7732999"}
```
We should not cut a release with known log spam.
I kept the build ids in Temporal as having any characters at all, but
when converted to a Deployment or Pod name, clean them to DNS only.
<!--- add/delete as needed --->
1. Closes #134
2. How was this tested:
Unit tests for the build id change
Checking logs on integration tests for the other changes. I'd like to
make the tests error if there are too many warnings, but the log level
is `info` on these, so I'm not sure how to make that work
3. Any docs updates needed?
<!--- update README if applicable
or point out where to update docs.temporal.io -->
…temporal-worker-controller into jlegrone/k8s-api-conventions
carlydf
approved these changes
Sep 10, 2025
carlydf
pushed a commit
that referenced
this pull request
Oct 20, 2025
…changes in #136 (#154) <!--- 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 updated documentation to reflect connectionRef and mutualTLSSecretRef changes in #136 ## Why? The current documentation is outdated
shashwatsuri
pushed a commit
to shashwatsuri/temporal-worker-controller
that referenced
this pull request
Apr 28, 2026
This PR updates the Temporal Worker Controller APIs to follow Kubernetes API conventions documented in https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md. ## What changed - Added standard `conditions` field to both status structs per k8s API conventions - Refactored `RampPercentage` in rollout spec from float to int - Changed connection references from plain strings to "ref" types (`connectionRef`, `mutualTLSSecretRef`) - Removed `Selector` field from `TemporalWorkerDeploymentSpec` (deployment selectors are now computed by the controller directly, avoiding possibility for misconfiguration) - Added validation kubebuilder annotations to several fields - Updated demo YAML examples to use new field names ## Breaking changes This requires updating existing YAML files due to field name changes and reinstalling the CRDs. ## Checklist 1. Closes temporalio#103 2. How was this tested: unit/integration tests should pass 3. Any docs updates needed?
shashwatsuri
pushed a commit
to shashwatsuri/temporal-worker-controller
that referenced
this pull request
Apr 28, 2026
…changes in temporalio#136 (temporalio#154) <!--- 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 updated documentation to reflect connectionRef and mutualTLSSecretRef changes in temporalio#136 ## Why? The current documentation is outdated
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR updates the Temporal Worker Controller APIs to follow Kubernetes API conventions documented in https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md.
What changed
conditionsfield to both status structs per k8s API conventionsRampPercentagein rollout spec from float to intconnectionRef,mutualTLSSecretRef)Selectorfield fromTemporalWorkerDeploymentSpec(deployment selectors are now computed by the controller directly, avoiding possibility for misconfiguration)Breaking changes
This requires updating existing YAML files due to field name changes and reinstalling the CRDs.
Checklist