semver-tool icon indicating copy to clipboard operation
semver-tool copied to clipboard

semver validate exit codes

Open trenslow opened this issue 2 years ago • 5 comments

Hi community,

I'm raising an issue to see if anyone else would be interested in having exit codes returned when using the 'validate' command.

Since this tool is built with CI in mind, it would be useful if a pipeline, pre-commit hook, etc. could be failed due to a non-zero exit code on version validation, rather than just echoing valid/invalid.

What does everyone else think? Is there some gotcha that I didn't consider? Maybe it could be controlled with a flag to start with, e.g. --exit-code, so as to not break pipelines currently using this tool.

Thanks for your consideration

trenslow avatar Aug 05 '22 08:08 trenslow

(I'm sure that I, somewhere, wrote a comment about this sort of change; but, for the life of me, I can't find it. So, here it goes again)

As of today, the semver-tool communicates results on stdout. stderr gets error messages and the return status is only used for errors (e.g. unknown options, malformed arguments, ..). It is relatively easy to write shell scripts that "convert" stdout text to return codes using test. However, outside of script contexts (i.e. execs from other tools, can't write shell functions to wrap validate, ..), a return code might be useful.

if [ $(semver validate 0.0.0) == 'valid' ]
then
. . .

One advantage of this scheme is that non-zero return codes mean there is an error and exceptional processing can be triggered. For CI pipelines, this might be aborting the build: the pipe itself is broken. And (for those who don't write CI pipeline tests!), coding errors don't get "swallowed". E.g. misspell "validate" and suddenly all versions are "invalid". Also, the code above makes the logic explicit.

Now, there are other points of view. See, for example, the man page for cmp. Also, see the @fsaintjacques comment to issue #61. While the comment suggests using the return value, this was not implemented and the subsequent PR #62 was accepted :-).

So, I see (for now), a few questions to discuss and resolve:

  1. Advantage of consistency and backward compatibility (use stdout exclusively) vs. open the door to simpler and sometimes more flexible return codes?
  2. What breaks? I think a lot breaks if non-zero codes start being used. A command line option seems clunky and about as verbose as the test syntax. An environment variable (hidden global variable that breaks stuff!)?
  3. If return codes are introduced, we need to define different codes for different situations: real errors vs results. Do we use negative values? For errors? For the results of compare? (e.g. -1,0,1)? Find and follow models from existing commands?
  4. If return codes are not introduced, maybe publish example wrappers?

ranger6 avatar Aug 05 '22 11:08 ranger6

One advantage of this scheme is that non-zero return codes mean there is an error and exceptional processing can be triggered. For CI pipelines, this might be aborting the build:

If the version isn't valid, I do indeed want to break the build. I can of course script this in my pipeline (or Bazel, or Make) but then I might as well just use the regex you provide and I don't need semver at all. I'd love to move away from the NPM provided semver (for about 100 reasons) but why not make this tool more like other *nix tools?

smoyer64 avatar Oct 09 '22 19:10 smoyer64

If the version isn't valid, I do indeed want to break the build. I can of course script this in my pipeline (or Bazel, or Make) but then I might as well just use the regex you provide and I don't need semver at all. I'd love to move away from the NPM provided semver (for about 100 reasons) but why not make this tool more like other *nix tools?

Yes, it could be that an invalid version string indicates a fatal error/bug in the build system. It could also mean, for example, that some versions (some libraries, legacy, ..) of software are not using semantic versioning and need to be handled differently.

As for being more *nix-like. Well, it's not clear. Searching around, I found a fair amount of information about standardized exit code values (e.g. https://tldp.org/LDP/abs/html/exitcodes.html). The common usage is for errors, not results; but there are exceptions (e.g. "cmp"). When errors and results are mixed, it pretty much becomes necessary to have some sort of explicit test to distinguish between an error (e.g. for "file not found", cmp returns "2") and a result (cmp returns "1" if file contents differ). For me, it's hard to see how things become shorter or simpler.

I still think that the questions posed above make sense and anyone motivated to provide a PR should address them.

ranger6 avatar Oct 12 '22 13:10 ranger6

@ranger6 - Agreed and I certainly didn't mean to imply that my use-case was THE use- case :) I also wasn't recommending this behavior for the other functions this program provides and was rather suggesting that failing a version check is indeed an error. I'm only using this to make sure that when our developers try to tag a release/pre-release, they've followed semantic versioning and while it kills me a little on the inside, I can keep validating using the NPM package.

smoyer64 avatar Oct 12 '22 19:10 smoyer64

No, I'm sure that you were not claiming your use case as "the way". And, adding an error exit code to "validate" does not necessarily imply adding it to other sub-commands: as I mentioned, it "opens the door". If the model for validate is to drive logic from exit values rather than stdout values, then why not also (for example) for "compare"?

But as I understand your (reasonable) point of view: "failing a version check is indeed an error". In your use case (someone not using semantic versioning as required), it is an error .. at the level of your build system logic.

For my example use case (support for legacy code), it's not an error. I would use the result (stdout or exit code) for branching logic. I would not want or expect the command to raise an exception that I'd catch to abort the build. In fact, if a real error condition was present (e.g. command not found because the PATH not set correctly), I would like a (fatal) error raised (and maybe caught/handled in an exceptional manner). As a practical matter, in Bash, the real error conditions get swallowed when they occur in "if", "while", "test" structures. So, it is hard to distinguish errors from non-semantic versions .. in Bash.

The good news and argument for going forward with the exit code proposal is that I believe there may not be so many situations where the logic would be broken (in Bash scripts). Mostly because the use of the "validate" sub-command as it operates today will mostly be in conditional expressions such as:

if [ $(semver validate $version) == 'valid' ]
then
    (handle valid semver)
else
    (handle invalid semver)
fi

Above, if "validate" is mis-spelled, or semver is not on the path; the else section will be executed (no exception raised!). So, if you add a non-zero exit code to "validate", the code still works.

Now the bad news:

validity=$(semver validate $version)

If one adds a non-zero exit code to "validate", an error will be raised. If I'm catching errors (e.g. trap ... ERR), I'll be getting a new fatal error.

To finish up: semver does consider a non-semantic version string to be an error when the sub-command expects a valid semantic version string. E.g. semver get major x.0 will return a non-zero exit code. It's just that the "validate" sub-command expects either valid or invalid strings. To abort a CI pipeline due to an invalid string is only slightly ugly:

semver get major $version > /dev/null

P.S. Thanks for the engagement!

ranger6 avatar Oct 13 '22 10:10 ranger6