Add Worker Deployment Versioning sample#48
Conversation
| module WorkerVersioning | ||
| class SomeActivity < Temporalio::Activity::Definition |
There was a problem hiding this comment.
| module WorkerVersioning | |
| class SomeActivity < Temporalio::Activity::Definition | |
| module WorkerVersioning | |
| module Activities | |
| class SomeActivity < Temporalio::Activity::Definition |
These should be in a module called Activities if they are in a non-main file with that name IMO
| TASK_QUEUE = 'worker-versioning' | ||
| DEPLOYMENT_NAME = 'my-deployment' |
There was a problem hiding this comment.
| TASK_QUEUE = 'worker-versioning' | |
| DEPLOYMENT_NAME = 'my-deployment' | |
| module Constants | |
| TASK_QUEUE = 'worker-versioning' | |
| DEPLOYMENT_NAME = 'my-deployment' | |
| end |
Not really required, but clearer IMO
| client = Temporalio::Client.connect( | ||
| 'localhost:7233', | ||
| 'default', | ||
| logger: logger |
There was a problem hiding this comment.
| logger: logger | |
| logger: |
Pedantic, but don't need to provide variable if named the same as the kw arg
| # them, we're demonstrating that the client remains version-agnostic. | ||
| auto_upgrade_workflow_id = "worker-versioning-versioning-autoupgrade_#{SecureRandom.uuid}" | ||
| auto_upgrade_execution = client.start_workflow( | ||
| 'AutoUpgrading', |
There was a problem hiding this comment.
| 'AutoUpgrading', | |
| :AutoUpgrading, |
I think you can use a symbol here (but if not, we need to fix it) and for other workflows you reference by name if you don't want to actually reference a class
| # Now we'll conclude all workflows. You should be able to see in your server UI that the pinned | ||
| # workflow always stayed on 1.0, while the auto-upgrading workflow migrated. | ||
| [auto_upgrade_execution, pinned_execution, pinned_execution_v2].each do |handle| | ||
| handle.signal('do_next_signal', 'conclude') |
There was a problem hiding this comment.
| handle.signal('do_next_signal', 'conclude') | |
| handle.signal(:do_next_signal, 'conclude') |
Pedantic, but also for signal names a symbol makes more sense (or let me know if it doesn't work)
| rescue StandardError => e | ||
| logger.error("Worker failed: #{e}") | ||
| raise |
There was a problem hiding this comment.
| rescue StandardError => e | |
| logger.error("Worker failed: #{e}") | |
| raise |
Can probably leave this off IMO (other samples do)
| require 'temporalio/workflow/definition' | ||
| require_relative 'activities' | ||
|
|
||
| module WorkerVersioning |
There was a problem hiding this comment.
| module WorkerVersioning | |
| module WorkerVersioning | |
| module Workflows |
For clarity IMO
| end | ||
|
|
||
| def execute | ||
| Temporalio::Workflow.logger.info('Changing workflow v1 started.') |
There was a problem hiding this comment.
Very pedantic, but trailing punctuation a bit inconsistent here
| workflow_versioning_behavior Temporalio::VersioningBehavior::AUTO_UPGRADE | ||
|
|
||
| def initialize | ||
| @signals = [] |
There was a problem hiding this comment.
Don't need to change anything, but if you'd like, you can remove this, change the signal method to (@signals ||= []) << signal and change wait condition to Temporalio::Workflow.wait_condition { @signals&.any? }
There was a problem hiding this comment.
I'll keep it as is. More obvious to me.
There was a problem hiding this comment.
I know we don't in other SDKs' samples (and it has bitten us), but in Ruby samples we're trying to have a test for every sample so it doesn't start breaking without us knowing. Even basic tests to make sure the workflows work is ideal, though this can be an opportunity to show how to test versioning if we want to.
There was a problem hiding this comment.
Added one (same as what I did in .net)
cretz
left a comment
cretz
left a comment
There was a problem hiding this comment.
LGTM. Small, non-blocking suggestion on the not-found except clause
2 participants
What was changed
Updating (adding, here) versioning samples in all langauges
Why?
Checklist
Closes
How was this tested: