auto-merge should accept non-conforming semver-derived version strings.

[hjdhjd]

First...this is a terrific Github action, and I look forward to continuing to see it evolve. That said, it does not flex sufficiently to deal with a variety of semver-derived version formats. You can say “hey, it’s not in the correct semver format”, and you’re right. :smile: But the reality is many developers (myself included) will use variations of semver for various reasons.

For example:

v1.2.3

is a very common version string. This creates a real problem for many users, and in glancing through your code:

https://github.com/ahmadnassri/action-dependabot-auto-merge/blob/544eb5ab8c52428999585e9cd0fc33fe6980e0c6/action/lib/parse.js#L41

you exit out when you can’t parse it to a canonical semver. I’d argue the right thing for the user is to attempt to parse it and if it’s one of the more common formats (e.g. v1.2.3), you should accept, not reject it. In short: if you can determine intent, the action should continue executing the action, not produce a warning and error out.

Otherwise, this terrific Github action is going to be of limited utility to a significant number of people because it doesn’t in fact auto-merge all the dependencies it should.

As an example of the above, setup-node, an incredibly common Github action that’s used pretty ubiquitously for many and it uses the v1.2.3 format:

https://github.com/actions/setup-node/releases

I saw this was discussed to some degree in #16 but I believe it warranted it’s own issue since the “fix” there is definitely not the expected behavior I, or I imagine many people, would expect here.

The right answer, I would argue is that it’s very clear what the version major, minor, and patch level is, and auto-merge should accept it, without complaining, and continue to execute the action.

[ahmadnassri]

I'm confused. in your example you mention v1.2.3 but that is a valid semver and this action does in fact parse and process that ...

do you have an example where it doesn't?

[hjdhjd]

Sure:

https://github.com/hjdhjd/homebridge-unifi-protect/pull/120/checks?check_run_id=1197356031

[hjdhjd]

And arguably, v1.2.3 isn’t a valid semver. v1 !== 1...technically. In my quick glance, it seems that your regex doesn’t seem to account for this and takes a more strict view, when I’m arguing that v1 === 1 should be the right answer. :smile:

[ahmadnassri]

I see the issue, your config is wrong:

update_type: semver:${TARGET}

should replace ${TARGET} with one of major, minor, patch

I wonder what in the docs lead you to this misunderstanding... and how to make the doc better?

[ahmadnassri]

as for the version number parsing, it would work for both v1.2.3 and 1.2.3 ... in fact here's the same pull request on this very repo working successfully https://github.com/ahmadnassri/action-dependabot-auto-merge/pull/28

[hjdhjd]

Thanks @ahmadnassri - and my comment on the regex was incorrect. I typoed testing it. :smile:

So the doc issue...

https://github.com/ahmadnassri/action-dependabot-auto-merge#defaults

I interpreted the line:

Where $TARGET is the target value from the action Inputs

literally, instead of thinking I needed to substitute in a value there.

[ahmadnassri]

yeah that section can do better explaining ... I'll keep this ticket open and tackle a better wording.

thanks for reporting it!

[hjdhjd]

I'd suggest updating error messages when you parse the auto-merge.yml file. If there's an invalid setting, it would be great if auto-merge could tell me about it. Saying it can't parse a title is misleading...and that led me down the wrong path. :smile:

[ahmadnassri]

indeed.

[ahmadnassri]

I'd also happily accept PRs 😉

[hjdhjd]

Wouldn't we all! :smile: I've already got my own backlog on my projects, but I'll take it on this weekend if you're willing to accept one. It's the least I could do.

[hjdhjd]

I noticed even in the example you shared - you still warn of an invalid semver:

https://github.com/ahmadnassri/action-dependabot-auto-merge/pull/28/checks?check_run_id=1197278591#step:3:47

If it's valid, it's valid. :smile:

[ahmadnassri]

wait wat ... that's not good ...

I'll have to investigate further

[hjdhjd]

cough regex cough

[ahmadnassri]

you were right, and I was dead wrong... also clearly was blind to that PR I referenced as example!!!

thanks again for reporting, I'll continue to work on the docs issue as well this weekend (or if you get to it sooner)

the fix is pushed and will be released automatically in a few seconds.

[github-actions[bot]]

:tada: This issue has been resolved in version 2.1.2 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket: