8000 finish deprecation of author in favour of authors array by denis-yuen · Pull Request #269 · dockstore/dockstore-cli · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

Merged
merged 5 commits into from
Dec 14, 2023

Conversation

denis-yuen
Copy link
Member
@denis-yuen denis-yuen commented Dec 6, 2023

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!

  • Check that you pass the basic style checks and unit tests by running ./mvnw clean install
  • If this PR is for a user-facing feature, create and link a documentation ticket for this feature (usually in the same milestone as the linked issue). Style points if you create a documentation PR directly and link that instead.

@denis-yuen denis-yuen self-assigned this Dec 6, 2023
@denis-yuen denis-yuen changed the title test this finish deprecation of authors in favour of array Dec 6, 2023
@denis-yuen denis-yuen changed the title finish deprecation of authors in favour of array finish deprecation of author in favour of authors array Dec 6, 2023
@denis-yuen
Copy link
Member Author

Supports dockstore/dockstore#5706

Copy link
codecov bot commented Dec 6, 2023

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (ec69dc0) 70.32% compared to head (a289f66) 70.19%.
Report is 3 commits behind head on release/1.15.0.

Files Patch % Lines
...ava/io/dockstore/client/cli/nested/ToolClient.java 25.00% 0 Missing and 3 partials ⚠️
...io/dockstore/client/cli/nested/WorkflowClient.java 0.00% 0 Missing and 1 partial ⚠️
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     
Flag Coverage Δ
bitbuckettests 9.83% <0.00%> (+<0.01%) ⬆️
confidentialtooltests 55.75% <20.00%> (+0.05%) ⬆️
confidentialworkflowtests 30.30% <0.00%> (-0.05%) ⬇️
nonconfidentialtests 32.37% <0.00%> (-0.02%) ⬇️
singularitytests 16.59% <0.00%> (+<0.01%) ⬆️
unittests 8.35% <0.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@denis-yuen denis-yuen requested review from a team, david4096, hyunnaye, svonworl, coverbeck and kathy-t and removed request for a team December 6, 2023 22:24
@denis-yuen denis-yuen marked this pull request as ready for review December 6, 2023 22:24
10000
@@ -810,7 +811,7 @@ public void handleInfo(String entryPath) {
description = "";
}

String author = container.getAuthor();
String author = container.getAuthors().isEmpty() ? "" : container.getAuthors().get(0).getName();
Copy link
Contributor

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?

Copy link
Member Author

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

  1. deletion of the old authors fields in the models and DB can wait
  2. for now, redirect the tests and queries

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thoughts @kathy-t ?

Copy link
Member Author

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

@denis-yuen denis-yuen changed the base branch from develop to release/1.15.0 December 13, 2023 20:50
Copy link

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

2 New issues
0 Security Hotspots
81.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@denis-yuen
Copy link
Member Author

Re-requesting due to dockstore/dockstore#5706 (comment)
i.e. this just relies on it being backwards compatible anyway

@@ -810,7 +811,7 @@ public void handleInfo(String entryPath) {
description = "";
}

String author = container.getAuthor();
Copy link
Contributor

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?

Copy link
Member Author
@denis-yuen denis-yuen Dec 14, 2023

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.

@denis-yuen denis-yuen requested a review from coverbeck December 14, 2023 15:30
@denis-yuen denis-yuen merged commit 071e320 into release/1.15.0 Dec 14, 2023
@denis-yuen denis-yuen deleted the feature/deprecate_author branch December 14, 2023 18:10
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.

4 participants
0