-
Notifications
You must be signed in to change notification settings - Fork 304
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
Automatically Convert name
Argument and registry
Argument to Lowercase in ImageSpec
#1881
Automatically Convert name
Argument and registry
Argument to Lowercase in ImageSpec
#1881
Conversation
For the databricks agent
[pull] master from flyteorg:master
Signed-off-by: Future Outlier <[email protected]>
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.
could you use post_init instead?
class ImageSpec():
...
...
def __post_init__(self):
self.name = self.name.lower()
self.registry = self.registry.lower()
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #1881 +/- ##
==========================================
- Coverage 55.06% 55.05% -0.01%
==========================================
Files 296 296
Lines 22250 22254 +4
Branches 3357 3358 +1
==========================================
+ Hits 12252 12253 +1
- Misses 9835 9838 +3
Partials 163 163
☔ View full report in Codecov by Sentry. |
Amazing advice, thanks a lot! |
Signed-off-by: Future Outlier <[email protected]>
…nto update-image-spec-name-to-lower
Signed-off-by: Future Outlier <[email protected]>
TL;DR
I recently started using "ImageSpec" and encountered an issue when I provided a name argument that wasn't in lowercase. Currently, the "ImageSpec" requires the
name
argument to be in lowercase, and it doesn't handle cases where users provide uppercase or mixed-case strings.To enhance user experience and reduce potential errors, I propose that we automatically convert the
name
argument to lowercase within the "ImageSpec". This minor adjustment will make the library more user-friendly and save users' time by preventing them from repeating this common mistake.Please consider this improvement for a future release. Looking forward to your feedback!
Screenshot
Type
Are all requirements met?
Tracking Issue
#1555