-
Notifications
You must be signed in to change notification settings - Fork 6
general bug fixes #92
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
Conversation
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 addresses various bug fixes that have accumulated in the main branch, focusing on function consolidation, null pointer safety, and configuration improvements.
Key changes include:
- Consolidating test workflow ID generation into a single shared function in the k8s package
- Adding null pointer safety checks for ramp timing operations to prevent panics
- Updating function signatures and parameter ordering for consistency
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
internal/temporal/worker_deployment_test.go | Updates test cases to use consolidated GetTestWorkflowID function with corrected parameter order |
internal/temporal/worker_deployment.go | Removes duplicate getTestWorkflowID function and adds temporalState parameter to GetTestWorkflowStatus |
internal/planner/planner_test.go | Adds test case for nil rampLastModifiedAt scenario and updates function call |
internal/planner/planner.go | Adds null check for rampLastModifiedAt and removes duplicate function |
internal/k8s/deployments.go | Adds consolidated GetTestWorkflowID function and fixes import ordering |
internal/demo/helloworld/temporal_worker_deployment.yaml | Updates pause durations in rollout strategy |
internal/demo/Dockerfile | Updates Go version from 1.23 to 1.24 |
internal/controller/worker_controller.go | Removes redundant defaulter call |
internal/controller/state_mapper.go | Adds explanatory comment for server bug fix |
internal/controller/genstatus.go | Adds temporalState parameter to GetTestWorkflowStatus call |
internal/controller/genplan.go | Adds empty string check for LastModifierIdentity |
helm/temporal-worker-controller/templates/crds/temporal.io_temporalworkerdeployments.yaml | Adds new schema fields |
Dockerfile | Adds internal/defaults directory to COPY instructions |
@jlegrone - so regarding this: this is technically a blocker for the controller as the controller needs this so that we can ramp a version properly; I already have a PR open on the server side which shall get completely rolled out in about 2 ish weeks; In terms of unblocking the controller, we do have the integ tests which shall have a non-nil value for this field thus Personally, I think we should keep the logic as-is rather than change it to account/handle server's error |
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.
two minor comments, mostly looks good to me but I haven't seen Jacob's comments yet
What was changed
main
Why?
Checklist
Closes
How was this tested: