-
Notifications
You must be signed in to change notification settings - Fork 3
finish deprecation of author in favour of authors array #269
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
Conversation
Supports dockstore/dockstore#5706 |
dockstore-client/src/main/java/io/dockstore/client/cli/nested/DepCommand.java
Outdated
Show resolved
Hide resolved
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## release/1.15.0 #269 +/- ##
====================================================
- Coverage 70.32% 70.19% -0.14%
+ Complexity 1055 1050 -5
====================================================
Files 47 47
Lines 6070 6069 -1
Branches 799 801 +2
====================================================
- Hits 4269 4260 -9
Misses 1466 1466
- Partials 335 343 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@@ -810,7 +811,7 @@ public void handleInfo(String entryPath) { | |||
description = ""; | |||
} | |||
|
|||
String author = container.getAuthor(); | |||
String author = container.getAuthors().isEmpty() ? "" : container.getAuthors().get(0).getName(); |
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.
If I'm reading this correctly, I didn't realize the CLI hadn't already been updated for the deprecation. Does this mean the 1.14 CLI will be broken against the 1.15 web service? If so, would it be better to not remove the deprecated fields from the web service until one more release?
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.
I think that yes, these two commands (updating and listing tools) will be broken.
Technically, if we go one more release, I'd probably break apart the webservice PR into two parts
- deletion of the old authors fields in the models and DB can wait
- for now, redirect the tests and queries
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.
thoughts @kathy-t ?
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.
Had a chat, we decided to make the webservice backwards compatible for existing 1.14 CLI users but merge most of the ui and cli changes so we don't have this problem in the 1.17 release
|
Re-requesting due to dockstore/dockstore#5706 (comment) |
@@ -810,7 +811,7 @@ public void handleInfo(String entryPath) { | |||
description = ""; | |||
} | |||
|
|||
String author = container.getAuthor(); |
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.
Based on the discussion, weren't you going to change this and call getAuthor()
after all?
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.
No, I thought the idea was that the 1.15 webservice would have both.
We would make the 1.15 cli use authors
instead of author, thus both the 1.14 and 1.15 cli would thus work with the 1.15 webservice. Then in 1.16 when we remove author
, we're hoping that users would be using the 1.15 cli and thus not notice anything.
Description
Supports dockstore/dockstore#5706
Adapts the CLI to accept the new authors list
Review Instructions
Build should pass mostly, bonus points for trying out the CLI list and update functionality in QA.
Issue
dockstore/dockstore#5467
Security
If there are any concerns that require extra attention from the security team, highlight them here.
Please make sure that you've checked the following before submitting your pull request. Thanks!
./mvnw clean install