fix: accept DSL versions from 1.0.0 up to the SDK's schema version#267
fix: accept DSL versions from 1.0.0 up to the SDK's schema version#267lornakelly wants to merge 1 commit into
Conversation
Signed-off-by: lornakelly <lornakelly88@gmail.com>
|
|
||
| import { schemaVersion } from '../../../package.json'; | ||
|
|
||
| const compareVersions = (a: string, b: string) => a.localeCompare(b, undefined, { numeric: true }); |
There was a problem hiding this comment.
In the Java SDK, we use a semver lib to compare versions. I'm not familiar with localeCompare, does it do the same?
There was a problem hiding this comment.
Not exactly the same, here the main difference is that semver knows about pre release tags so knows 1.0.3-alpha < 1.0.3, localCompare treats it the opposite because it is compares the numbers and sees 1.0.3 as the maximum but then assumes, because it is a longer string, it is greater than the allowed version
I went with the simpler built in localeCompare as I thought it overkill to add a new package dependency for this because I saw in package.json that the schemaVersion field is in the format major.minor.patch, its the version field that has the tag which we are not comparing https://github.com/serverlessworkflow/sdk-typescript/blob/main/package.json#L3
What I didnt take into account though is do people add tags in the workflow document dsl field?, so something like dsl: 1.0.3-alpha? If that is the case I would need to change this to use semver instead as with localeCompare that would get rejected today
I could also change it if you would rather consistency with Java sdk?
There was a problem hiding this comment.
I agree with @ricardozanini, I'd rather use semver as well. Could you please look at my comment in the issue and align your implementation with it?
(reasoning: if the schema was 2.0.0, which breaks with 1.0.0, the current code would still show a green ligth)
There was a problem hiding this comment.
Yes will do, thanks for the feedback
|
|
||
| import { schemaVersion } from '../../../package.json'; | ||
|
|
||
| const compareVersions = (a: string, b: string) => a.localeCompare(b, undefined, { numeric: true }); |
There was a problem hiding this comment.
I agree with @ricardozanini, I'd rather use semver as well. Could you please look at my comment in the issue and align your implementation with it?
(reasoning: if the schema was 2.0.0, which breaks with 1.0.0, the current code would still show a green ligth)
|
I forgot to mention, please also bump the version of the package to |
3 participants
Closes #266
What this PR does / why we need it:
Fixes DSL version validation rejecting valid older workflows. The hook compared the workflow's dsl against the SDK's
schemaVersion(1.0.3) with strict equality (!==), so only accepted workflows with dsl: "1.0.3" and rejected 1.0.0/1.0.1/1.0.2Changes:
schemaVersion