Story #3504

[SDKs] Clients are compatible with #3036

Added by Tom Clegg about 5 years ago. Updated about 5 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
SDKs
Target version:
Start date:
08/07/2014
Due date:
% Done:

100%

Estimated time:
(Total: 2.00 h)
Story points:
2.0

Subtasks

Task #3631: Review 3504-clients-compatible-with-3036ResolvedPeter Amstutz

Task #3507: Update clients to permit arvados uuids for CollectionsResolvedPeter Amstutz

Task #3520: Review 3036-mutable-collectionsResolvedPeter Amstutz


Related issues

Related to Arvados - Story #3036: [API] Use regular uuids instead of content hashes to identify collectionsResolved08/07/2014

Associated revisions

Revision a15f61ae
Added by Tom Clegg about 5 years ago

Merge branch '3036-mutable-collections' refs #3504

Revision accaf5ae (diff)
Added by Tom Clegg about 5 years ago

Do not try to predict uuid of new collection. refs #3261 refs #3504

Revision 1966eff7
Added by Peter Amstutz about 5 years ago

Merge branch '3504-clients-compatible-with-3036' refs #3504

Revision 0bbd1138 (diff)
Added by Peter Amstutz about 5 years ago

Display first file in collection if name field is nil or empty. refs #3504

Revision d85ec3b1 (diff)
Added by Peter Amstutz about 5 years ago

Display first file in collection if name field is nil or empty. refs #3504

Revision a0b0947b (diff)
Added by Peter Amstutz about 5 years ago

With new apiserver, after upload, arv-put prints portable data hash instead of
collection uuid for compatibility with crunch. refs #3504

Revision 70e7bc03 (diff)
Added by Tom Clegg about 5 years ago

Remove uuid-guessing code from "combine selected files". refs #3504

History

#1 Updated by Tim Pierce about 5 years ago

  • Category set to SDKs

#2 Updated by Peter Amstutz about 5 years ago

  • Assigned To set to Peter Amstutz

#3 Updated by Tom Clegg about 5 years ago

  • Status changed from New to In Progress

#4 Updated by Tom Clegg about 5 years ago

At 2769713

Workbench
  • Reviving tags might be a good idea in general but please resist further creep in this story/branch. Clicking a tag on the "show project" page takes me to a list of other collections, but they're surrounded by a bunch of indigestible stuff like head/tail/link_class, a "create link" button, more UUIDs. I'd rather un-hyperlink the tags until we have a decent target to link to (e.g., a post-#3149 search dialog).
  • On collections#index, remove the "storage" column in thead to match tbody.
  • The tag lookup code at the end of ProjectsController#load_contents_objects seems to duplicate existing functionality provided by preload_links_for_objects. I think you just need to call preload_links_for_objects(@objects) in the controller, and links_for_object(object).select{|link|link.name=='tag'} in the view.
  • I can't quite figure out what the change in apps/workbench/app/views/application/_show_attributes.html.erb is for. It seems to have the effect of restricting what can be achieved with @attribute_sortkey in ArvadosBase, but I haven't yet thought of a reason why.
  • In Collection#portable_data_hash, it might be safer to return self[:uuid] rather than self.uuid if self[:portable_data_hash] is nil (and similar in #uuid). If both are nil, returning nil seems more appropriate than going OOM or hitting an arbitrary recursion depth limit.
  • Collection#attribute_editable? looks like it returns true for 'manifest_text' which is technically wrong if still talking to a pre-#3036 API server. I don't know of anywhere in Workbench that actually acts differently either way though, so it's probably not worth bothering with.
Python
  • CollectionReader __init__ expects md5, needs to accept regular UUID as well

(Tests aren't passing, but I'm pretty sure you already know that.)

#5 Updated by Peter Amstutz about 5 years ago

Tom Clegg wrote:

At 2769713

Workbench
  • Reviving tags might be a good idea in general but please resist further creep in this story/branch. Clicking a tag on the "show project" page takes me to a list of other collections, but they're surrounded by a bunch of indigestible stuff like head/tail/link_class, a "create link" button, more UUIDs. I'd rather un-hyperlink the tags until we have a decent target to link to (e.g., a post-#3149 search dialog).

Agreed that the links index page is not very nice. However I'm going to leave the code it because it is still at least somewhat useful and it would be more effort to rip it out at this point.

  • On collections#index, remove the "storage" column in thead to match tbody.

Fixed.

  • The tag lookup code at the end of ProjectsController#load_contents_objects seems to duplicate existing functionality provided by preload_links_for_objects. I think you just need to call preload_links_for_objects(@objects) in the controller, and links_for_object(object).select{|link|link.name=='tag'} in the view.

Fixed.

  • I can't quite figure out what the change in apps/workbench/app/views/application/_show_attributes.html.erb is for. It seems to have the effect of restricting what can be achieved with @attribute_sortkey in ArvadosBase, but I haven't yet thought of a reason why.

Late-night attempt at improving the look of the details pages, and not aware of @attribute_sortkey. Reverted.

  • In Collection#portable_data_hash, it might be safer to return self[:uuid] rather than self.uuid if self[:portable_data_hash] is nil (and similar in #uuid). If both are nil, returning nil seems more appropriate than going OOM or hitting an arbitrary recursion depth limit.

Fixed.

  • Collection#attribute_editable? looks like it returns true for 'manifest_text' which is technically wrong if still talking to a pre-#3036 API server. I don't know of anywhere in Workbench that actually acts differently either way though, so it's probably not worth bothering with.

Left it alone.

Python
  • CollectionReader __init__ expects md5, needs to accept regular UUID as well

Fixed.

(Tests aren't passing, but I'm pretty sure you already know that.)

Tests pass now.

#6 Updated by Tom Clegg about 5 years ago

Peter Amstutz wrote:

Agreed that the links index page is not very nice. However I'm going to leave the code it because it is still at least somewhat useful and it would be more effort to rip it out at this point.

The changes to the links view etc. are harmless, I'm just asking for the following change, to avoid luring the poor users into a construction zone:
  • -          <%= link_to tag_link.name, controller: "links", filters: [["link_class", "=", "tag"], ["name", "=", tag_link.name]].to_json %>
    +          <%= tag_link.name %>
    
  • On collections#index, remove the "storage" column in thead to match tbody.

Fixed.

Great. Should make the remaining column widths add up to 100% though. (Add the spare 12% to the last (tags) column?)

Rest of the fixes look good.

Just a couple of new things:

In CollectionsController#choose we concatenate @collections + @objects. If I'm following correctly, old API will put nothing in @collections while new API will put nothing in @objects so this is just a way to "do whatever's appropriate for the current API version" without actually deciding which version it is. Is that right? (Either way I think a quick comment would be a good investment. "Only one of @objects and @collections will be non-empty, depending on API version, see #3036. FIXME: remove 'name links' stuff here after #3036 is deployed everywhere."?)

(Mentioned in #3036 but seems to be fixed differently in 3504 branch) In sdk/python/arvados/commands/put.py we set owner_uuid during collections().create() and then update it to the same value in collections().update(). Should probably remove owner_uuid from the update call, unless of course it's needed for some reason I'm not thinking of.

Thanks.

#7 Updated by Peter Amstutz about 5 years ago

Tom Clegg wrote:

The changes to the links view etc. are harmless, I'm just asking for the following change, to avoid luring the poor users into a construction zone:
  • [...]

Fixed

Great. Should make the remaining column widths add up to 100% though. (Add the spare 12% to the last (tags) column?)

Fixed

In CollectionsController#choose we concatenate @collections + @objects. If I'm following correctly, old API will put nothing in @collections while new API will put nothing in @objects so this is just a way to "do whatever's appropriate for the current API version" without actually deciding which version it is. Is that right? (Either way I think a quick comment would be a good investment. "Only one of @objects and @collections will be non-empty, depending on API version, see #3036. FIXME: remove 'name links' stuff here after #3036 is deployed everywhere."?)

Added comment.

(Mentioned in #3036 but seems to be fixed differently in 3504 branch) In sdk/python/arvados/commands/put.py we set owner_uuid during collections().create() and then update it to the same value in collections().update(). Should probably remove owner_uuid from the update call, unless of course it's needed for some reason I'm not thinking of.

Fixed. Juggling changes between branches is tricky.

#8 Updated by Tom Clegg about 5 years ago

At 73bbd41...

  • The "add tag" button doesn't actually work on the projects#show→collections tab. I think it successfully adds a tag with head_uuid=uuid_of_name_link, which succeeds, but that tag doesn't get loaded when I hit Refresh.
  • The "add tag" button appears in a project where I have no write permission.

Other fixes look good

#9 Updated by Tom Clegg about 5 years ago

(If it's not already removed) I think this is obsolete now in projects.css.scss

.removable-tag:hover {
    cursor: pointer;
}
\ No newline at end of file

#10 Updated by Peter Amstutz about 5 years ago

da8b740

  1. Removed tag rendering code from project rows.
  2. Merged a few small fixes from 3036
  3. Tests pass

#11 Updated by Tom Clegg about 5 years ago

All that looks good, and I haven't found any further problems. Thanks!

#12 Updated by Peter Amstutz about 5 years ago

  • Status changed from In Progress to Resolved

Also available in: Atom PDF