To prevent dependabot-core from failing when the incorrect release tag is created for a release, adding a rescue statement

[honeyankit]

When the user creates a wrong release tag, the dependabot is not handling the exception and failing. The fix in this PR will catch such exception and will return nothing for such cases.

Error:

updater | ERROR <job_446384276> Error processing yarl (ArgumentError)
updater | ERROR <job_446384276> Malformed version number string 1.8.0..failed.release.attempt

• Error logs before fix: failed-argument-error.log
• Logs after fix: fixed-agrement-error.log

---

Original issue: https://github.com/github/dependabot-updates/issues/2776

[deivid-rodriguez]

Hei @honeyankit! Nice to get rid of all these Python errors 🎉.

However I believe this rescue might not be the best solution.

While it correctly ignores this case, it may ignore many other things, including programming mistakes!

I think even if with decide to go with a silencing rescue, it should be more scoped to the exact place where this issue is happening.

How about trying to fix the actual culprit instead? I think you already identified the issue, this tag is getting incorrectly flagged as correct? by our version regexp. And that's why we later try to instantiate a Python::Version out of it.

I think the double dash is suspicious, I think the pattern may be unintentionally allowing that. Does this patch fix things?

diff --git a/python/lib/dependabot/python/version.rb b/python/lib/dependabot/python/version.rb
index 525be66d6..0290de9f0 100644
--- a/python/lib/dependabot/python/version.rb
+++ b/python/lib/dependabot/python/version.rb
@@ -16,7 +16,7 @@ module Dependabot
 
       # See https://peps.python.org/pep-0440/#appendix-b-parsing-version-strings-with-regular-expressions
       VERSION_PATTERN = 'v?([1-9][0-9]*!)?[0-9]+[0-9a-zA-Z]*(?>\.[0-9a-zA-Z]+)*' \
-                        '(-[0-9A-Za-z-]+(\.[0-9a-zA-Z-]+)*)?' \
+                        '(-[0-9A-Za-z]+(\.[0-9a-zA-Z]+)*)?' \
                         '(\+[0-9a-zA-Z]+(\.[0-9a-zA-Z]+)*)?'
       ANCHORED_VERSION_PATTERN = /\A\s*(#{VERSION_PATTERN})?\s*\z/.freeze
 

[mattt]

How about trying to fix the actual culprit instead? I think you already identified the issue, this tag is getting incorrectly flagged as correct? by our version regexp. And that's why we later try to instantiate a Python::Version out of it.

The linked PEP440 doc provides a regex pattern for matching version identifiers, but it's not obvious to me how that relates to the VERSION_PATTERN we use here. You might try copying that verbatim to see if that solves the problem at hand.

[deivid-rodriguez]

I had the same thought, both regexp don't look the same to me. Anyways, I suggested a focused patch to what we currently have, because it seems "mostly" correct already (as per the different tests cases in the Version class unit specs).

I also checked that this version is not correct according to Python itself with

$ pyenv exec python
Python 3.10.5 (main, Aug 12 2022, 10:44:57) [GCC 9.4.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from packaging.version import Version
>>> Version("v1.8.0--failed-release-attempt")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/.pyenv/versions/3.10.5/lib/python3.10/site-packages/packaging/version.py", line 298, in __init__
    raise InvalidVersion("Invalid version: '{0}'".format(version))
packaging.version.InvalidVersion: Invalid version: 'v1.8.0--failed-release-attempt'

Also here's a little extra spec for this case!

diff --git a/python/spec/dependabot/python/version_spec.rb b/python/spec/dependabot/python/version_spec.rb
index 58ea79885..51f2cd33d 100644
--- a/python/spec/dependabot/python/version_spec.rb
+++ b/python/spec/dependabot/python/version_spec.rb
@@ -48,6 +48,11 @@ RSpec.describe Dependabot::Python::Version do
         let(:version_string) { "1.0.0+abc 123" }
         it { is_expected.to eq(false) }
       end
+
+      context "that includes two dashes" do
+        let(:version_string) { "v1.8.0--failed-release-attempt" }
+        it { is_expected.to eq(false) }
+      end
     end
   end

[mattt]

I had the same thought, both regexp don't look the same to me. Anyways, I suggested a focused patch to what we currently have, because it seems "mostly" correct already (as per the different tests cases in the Version class unit specs).

A focused change sounds good to me. And thanks for verifying the existing behavior in Python.

If we do change the regex, I'd really like to see a comment explaining that our regex has diverged from PEP440 (bonus points if we can articulate why).

[deivid-rodriguez]

For what it's worth the Python regexp never includes dashes in alphanumeric character classes, so this change at least brings us closer to the "upstream regexp".

[honeyankit]

@deivid-rodriguez thanks for reviewing the PR and providing the suggestion. My initial thought was to modify the regex and remove the double-dash, since single dash is acceptable with for the python version. I tried version check with some other python module and it came out correct making me believe that version is correct, so changed my direction and use rescue to catch the exception.