Update compatibility adapter to support new enum value #1337
Conversation
Shaddoll
requested review from
Groxx,
shijiesheng,
agautam478,
jakobht,
3vilhamster,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted filesContinue to review full report in Codecov by Sentry.
|
| @@ -364,6 +364,8 @@ func CancelExternalWorkflowExecutionFailedCause(t *shared.CancelExternalWorkflow | |||
| switch *t { | |||
| case shared.CancelExternalWorkflowExecutionFailedCauseUnknownExternalWorkflowExecution: | |||
| return apiv1.CancelExternalWorkflowExecutionFailedCause_CANCEL_EXTERNAL_WORKFLOW_EXECUTION_FAILED_CAUSE_UNKNOWN_EXTERNAL_WORKFLOW_EXECUTION | |||
| case shared.CancelExternalWorkflowExecutionFailedCauseWorkflowAlreadyCompleted: | |||
| return apiv1.CancelExternalWorkflowExecutionFailedCause_CANCEL_EXTERNAL_WORKFLOW_EXECUTION_FAILED_CAUSE_WORKFLOW_ALREADY_COMPLETED | |||
| } | |||
| panic("unexpected enum value") | |||
There was a problem hiding this comment.
Not as part of this PR but we should revisit the idea of panic'ing here. This makes the client not forward-compatible. Especially for the fields that are only used for informational purposes we don't need to be strict like this.
@Groxx WDYT?
There was a problem hiding this comment.
personally, I think falling back to a default value is a safe option and it makes sense
There was a problem hiding this comment.
Since we have a decent "unknown" value, yea - unknown is probably fine as a fallback. unrecognized is unknown, and should probably be treated the same. it's probably a safe option here, though some kind of "non-fatal err return value" would let us detect it (otherwise this'll be a silent fallback, possibly hiding who knows how many flaws).
the main thing that makes me actually prefer panics is that they tend to reliably fail tests. assert.Error(t, err) will happily ignore return errors.New("impossible: unrecognized enum") despite that being clearly intended to be a "must fail tests" value. they also tend to be auto-categorized as "severe problem, alert oncall" because panics simply shouldn't be happening.
so mistakes tend to be noticed and fixed faster.
they are rather clearly more dangerous... but bad data is often worse, as it's so hard to fix and tends to go unnoticed for a long time.
anyway.
personally I think things like this should be immediately alerted, so we probably need some kind of universally-recognized "this is a flaw that we could not prevent at compile time" marker. historically I've used ProgrammerError(..) types, and hooked them up to always emit a metric when they're returned anywhere. it worked pretty well.
we would need to add those checks and switch to error wrapping (so we don't lose the type) and also exclude them from all assert.Error checks to keep them as identifiable as panics tho.
What changed?
Why?
How did you test it?
unit tests
Potential risks