elastic / snyk-github-issue-creator

CLI tool for creating GitHub issues from Snyk project issues
5 stars 6 forks source link

Unable to handle non-semver compliant version strings #24

Closed gcmurphy closed 1 year ago

gcmurphy commented 2 years ago

In cases where the package is not semver compliant we are not able reconcile state.

The affected line of code - https://github.com/elastic/snyk-github-issue-creator/blob/main/cli/utils.js#L30

A minimal reproducer:

const semver = require('semver')

const compareVersions = (a, b) => 
  semver.lt(a, b) ? 1 : semver.gt(a, b) ? -1 : 0;

let foo = compareVersions('1.2.3.4', '1.2.3.5');
console.log(foo);

Resulting error:

demo/node_modules/semver/classes/semver.js:38
      throw new TypeError(`Invalid Version: ${version}`)
      ^

TypeError: Invalid Version: 1.2.3.4
    at new SemVer (demo/node_modules/semver/classes/semver.js:38:13)
    at compare (demo/node_modules/semver/functions/compare.js:3:3)
    at Object.lt (demo/node_modules/semver/functions/lt.js:2:29)
    at compareVersions (demo/index.js:4:10)
    at Object.<anonymous> (demo/index.js:6:11)
    at Module._compile (node:internal/modules/cjs/loader:1149:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1203:10)
    at Module.load (node:internal/modules/cjs/loader:1027:32)
    at Module._load (node:internal/modules/cjs/loader:868:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)

There are unfortunately libraries that do this which I have to deal with.. jackson-databind for example :cry:.

watson commented 2 years ago

It should be fixed now and released to npm as version 1.8.0. Hopefully it supports all variations of a four-part version now. Thanks for reporting this issue 🙏

gcmurphy commented 1 year ago

@watson I just encountered another different variation of this problem with this package. I'll re-open the issue but feel free to close it as I understand how this may become a case of whack-a-mole to support all the weird versioning schemes.

watson commented 1 year ago

@gcmurphy As far as I can see this is because there's only two parts to the version right? I'll make an update that supports it so that the patch or even the minor is "missing" (in which case I'll just default them to zero).

watson commented 1 year ago

I've now released v1.9.0 to npm with a less strict version string parser. So it now supports both 1 and 1.2 and even v1.2.3 and a few others. Feel free to reopen the issue if I've missed something. If it turns out that this still doesn't cover everything, I think the next cause of action is to simply fall back to not sorting the versions that it can't parse (instead of failing as it does now).