Skip to content

fix: converters reject signed resource values to match validator behavior#1588

Open
immanuwell wants to merge 1 commit into
argoproj-labs:mainfrom
immanuwell:fix-converter-signed-values
Open

fix: converters reject signed resource values to match validator behavior#1588
immanuwell wants to merge 1 commit into
argoproj-labs:mainfrom
immanuwell:fix-converter-signed-values

Conversation

@immanuwell
Copy link
Copy Markdown
Contributor

Pull Request Checklist

Description of PR

Currently, _convert_decimal_units and _convert_binary_units accept signed values like -500m or +1Gi due to [+-]? in their regex patterns. The validators (_validate_decimal_units, _validate_binary_units) correctly reject these, so there's a mismatch - same input, different outcome depending on which function you hit first.

This PR removes [+-]? from both converter patterns so they're consistent with validators. Also fixes _merge_dicts raising bare Exception instead of ValueError.

Reproduce:

from hera.workflows.converters import _convert_decimal_units, _convert_binary_units
from hera.workflows.validators import _validate_decimal_units

_validate_decimal_units("-500m")   # raises ValueError (correct)
_convert_decimal_units("-500m")    # returns -0.5 (bug)

_convert_binary_units("-512Mi")    # returns -536870912.0 (bug)

Negative resource values aren't valid in Kubernetes anyway, so rejecting them in both layers makes sense.

…vior

Signed-off-by: immanuwell <pchpr.00@list.ru>
@immanuwell immanuwell force-pushed the fix-converter-signed-values branch from 5be510d to 3ffab33 Compare May 28, 2026 18:31
@codecov
Copy link
Copy Markdown

codecov Bot commented May 28, 2026

Codecov Report

❌ Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 80.7%. Comparing base (8f8a626) to head (3ffab33).

Files with missing lines Patch % Lines
src/hera/workflows/resources.py 0.0% 1 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff          @@
##            main   #1588   +/-   ##
=====================================
  Coverage   80.7%   80.7%           
=====================================
  Files         62      62           
  Lines       5081    5081           
  Branches     771     771           
=====================================
  Hits        4101    4101           
  Misses       846     846           
  Partials     134     134           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant