-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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: add jina version info to docker image name #1341
Conversation
ae9d7fc
to
5d7440e
Compare
5d7440e
to
2e85c70
Compare
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.
LGTM
@cristianmtr seems this integration test is broken
|
Let's fix the integration test and we are good to merge |
f7b45c2
2e85c70
to
f7b45c2
Compare
Oh missed that . Submitted a fix and rerunning now |
Codecov Report
@@ Coverage Diff @@
## master #1341 +/- ##
==========================================
- Coverage 83.37% 83.02% -0.36%
==========================================
Files 101 103 +2
Lines 6705 6814 +109
==========================================
+ Hits 5590 5657 +67
- Misses 1115 1157 +42
Continue to review full report at Codecov.
|
Integration test fixed now. But the codecov/project failed |
@@ -68,7 +68,7 @@ def remove_control_characters(s): | |||
|
|||
|
|||
def safe_url_name(s): | |||
return s.lower().replace('-', '--').replace('_', '__').replace(' ', '_') | |||
return s.lower().replace('_', '__').replace(' ', '_') |
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.
is there a unittest on what is expected here? why did we remove that part?
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.
It was never used before and it was unnecessary (as discussed with @hanxiao ). The unittest in https://github.com/jina-ai/jina/pull/1341/files#diff-e63749458c2c70804a69ac37c0a560e869b070408233b988a219514a8df6cf31R55 covers it (the name format)
@cristianmtr i guess you can ignore the coverage decrease for the time being, it's not related to your PR, it's related to the way we setup codecov. I'll look into this at night. |
-
being replaced with--