-
Notifications
You must be signed in to change notification settings - Fork 105
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(UserContainer): Fix a small bug in ImagePullPolicy mapper and add tests for error cases #963
fix(UserContainer): Fix a small bug in ImagePullPolicy mapper and add tests for error cases #963
Conversation
Signed-off-by: KengoA <20113339+KengoA@users.noreply.github.com>
Signed-off-by: KengoA <20113339+KengoA@users.noreply.github.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #963 +/- ##
=====================================
Coverage 81.7% 81.7%
=====================================
Files 54 54
Lines 4208 4208
Branches 889 889
=====================================
+ Hits 3439 3441 +2
+ Misses 574 572 -2
Partials 195 195 ☔ View full report in Codecov by Sentry. |
# some users might submit the policy without underscores | ||
**{ipp.value.lower().replace("_", ""): ipp for ipp in ImagePullPolicy}, | ||
# some users might submit the policy in lowercase | ||
**{ipp.name.lower(): ipp for ipp in ImagePullPolicy}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These were not accessing the intended values and created duplicate key-value pairs.
Reference:
# src/hera/workflows/models/io/k8s/api/core/v1.py
class ImagePullPolicy(Enum):
always = "Always"
never = "Never"
if_not_present = "IfNotPresent"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good find, thanks! cc @flaviuvadan JFYI 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! 🚀
# some users might submit the policy without underscores | ||
**{ipp.value.lower().replace("_", ""): ipp for ipp in ImagePullPolicy}, | ||
# some users might submit the policy in lowercase | ||
**{ipp.name.lower(): ipp for ipp in ImagePullPolicy}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good find, thanks! cc @flaviuvadan JFYI 👀
Pull Request Checklist
Description of PR
When checking for string values of
image_pull_policy
in theUserContainer
class, the current implementation was adding redundant items in thepolicy_mapper
dict as it was confusing thename
andvalue
properties.This PR simplifies this logic while covering the same use cases, and adds tests for normal and error cases.