Arvados

New version first commit: https://dev.arvados.org/projects/arvados-workbench-2/repository/revisions/37f3a8d7a7ce05a15fbb9219763b46ba1c250976
Test run: developer-tests-workbench2: #366

Created mountainduck bookmark files generator

Reviewing arvados-workbench2|37f3a8d

• As you can see on developer-tests-workbench2: #366 /console -- there're some problems with the cypress test environment.
• On the Cypress test, could we check that the downloaded .duck file's contents is correct instead of just checking that its size is greater than 50?
• At file src/store/collections/collection-info-actions.ts
  • Line 38, the collection's name that seems to be retrieved is the one that's being displayed on the collection panel. The problem I'm seeing with this approach is that the "webdav/S3 dialog" can be called from within a context menu inside a project listing, so in that case it will be retrieving an incorrect name if the user selected another collection previously. You can reproduce the issue doing:
    • Create a 'Collection A' collection inside the home project.
    • Create a 'Collection B' collection inside the home project.
    • From within the home project, click on the 'Collection A' collection.
    • Go back to the home project.
    • Right-click on the 'Collection B' collection, ask for the WebDav/S3 dialog and download the mountain duck file.
    • The downloaded file will have the 'Collection A' name.
  • Also line 38: I don't think overriding the typing system by casting to any is convenient.
• At file src/views-components/webdav-s3-dialog/webdav-s3-dialog.tsx
  • Line 72: Can you explain why you split and re-join the the returned string? If you're providing the template's string on the code, it shouldn't be necessary to do that kind of "cleaning".
  • Line 201: The button's label is not very clear what it downloads. Do you agree that changing it to 'Download Cyber/Mountain Duck bookmark' would be better?
• Also, have you tried the generated bookmark file? It doesn't work for me on arvbox and ce8i5. I believe it should also work using Cyberduck (free tool from the same team) because I get the same error message when trying both: DNS lookup for davs://user@IP_or_hostname failed.... Some possible problems are:
  • Username, protocol and port number get included on the 'hostname' field.
  • Port number is hardcoded to 443 (it can be different, for example arvbox uses a different one)
  • Password (token) doesn't get included on the file.

This is the .duck file that I got generated with CyberDuck Mac version 8.7.5:

<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
    <key>Protocol</key>
    <string>davs</string>
    <key>Provider</key>
    <string>iterate GmbH</string>
    <key>UUID</key>
    <string>ef96de3e-1bf7-4204-9f60-e2c035a338c1</string>
    <key>Hostname</key>
    <string>ce8i5-4zz18-mx9loldsvg238gk.collections.ce8i5.arvadosapi.com</string>
    <key>Port</key>
    <string>443</string>
    <key>Username</key>
    <string>ldipenti</string>
    <key>Path</key>
    <string>/</string>
    <key>Access Timestamp</key>
    <string>1618242186076</string>
</dict>
</plist>

Note that the Hostname key is populated with just the collection's hostname, and not the entire davs://ldipenti...@ URL.
The password isn't included (although somehow it gets saved and isn't requested), so I'm not sure if we could avoid the user having to enter the password (token) manually. Maybe Cyberduck's docs have some information about that.

Reviewing changes made on arvados-workbench2|c26242a

Although changes in .duck files made the bookmark work for clusters like ce8i5, it still has the issue of having the 443 port hardcoded and not supporting keep-web collection urls with non-standard ports: for example, if the collection's url is something like https://collection-uuid.hostname:9004, the .duck file's hostname field will be filled with "collection-uuid.hostname:9004" as a hostname, producing a DNS resolution error.

New version first commit: https://dev.arvados.org/projects/arvados-workbench-2/repository/revisions/645ea9356ef315154681d093cd6a42c1a8984e12
Test run: developer-tests-workbench2: #371

Fixed duck file added better tests

Tests are failing the same way as described on #note-3: developer-tests-workbench2: #371 /console

New version first commit: https://dev.arvados.org/projects/arvados-workbench-2/repository/revisions/eaf76a93400029d3d788264b829bf878aad1f661
Test run: developer-tests-workbench2: #381

Cypress upgrade to 3.6 in order to fix electron download issue

Please remove the it.only() call and re-run tests.

New version first commit: https://dev.arvados.org/projects/arvados-workbench-2/repository/revisions/0569677b7c4d4a907950b6dfc6218eabaa2aad59
Test run: developer-tests-workbench2: #384

Removed only

Reviewing arvados-workbench2|0569677b

Lucas Di Pentima wrote:

Reviewing arvados-workbench2|37f3a8d

• At file src/store/collections/collection-info-actions.ts
  • Line 38, the collection's name that seems to be retrieved is the one that's being displayed on the collection panel. The problem I'm seeing with this approach is that the "webdav/S3 dialog" can be called from within a context menu inside a project listing, so in that case it will be retrieving an incorrect name if the user selected another collection previously. You can reproduce the issue doing:
    • Create a 'Collection A' collection inside the home project.
    • Create a 'Collection B' collection inside the home project.
    • From within the home project, click on the 'Collection A' collection.
    • Go back to the home project.
    • Right-click on the 'Collection B' collection, ask for the WebDav/S3 dialog and download the mountain duck file.
    • The downloaded file will have the 'Collection A' name.

The above issue keeps happening.

• Also line 38: I don't think overriding the typing system by casting to any is convenient.

This wasn't addressed, is there a reason why?

• At file src/views-components/webdav-s3-dialog/webdav-s3-dialog.tsx
  • Line 72: Can you explain why you split and re-join the the returned string? If you're providing the template's string on the code, it shouldn't be necessary to do that kind of "cleaning".

This wasn't addressed also, why do you split and then join a string by newline characters if the string is already provided in the code?

• Line 201: The button's label is not very clear what it downloads. Do you agree that changing it to 'Download Cyber/Mountain Duck bookmark' would be better?

The button still states 'download config' on its label, do you think that's a clear enough name?

• The cypress test looks good. The XML file decoding code seems a little convoluted to me, it would be nice to have at least a comment explaining what is doing, for future readers.
• I think you should commit the updated yarn.lock file.
• The non-default port fix seems to be correct, but there's also a related issue with hostnames: When wildcard hostnames aren't set and only IPs are used (for example, the arvbox use case), the bookmark file should include a Path key with the value /c=<collection uuid or pdh> as described on https://doc.arvados.org/v2.1/api/keep-webdav.html

Here's an example that works on my arvbox instance:

<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
    <key>Protocol</key>
    <string>davs</string>
    <key>Provider</key>
    <string>iterate GmbH</string>
    <key>UUID</key>
    <string>x240b-4zz18-cx9p60347rmsyoi</string>
    <key>Hostname</key>
    <string>10.1.1.32</string>
    <key>Port</key>
    <string>9004</string>
    <key>Username</key>
    <string>user</string>
    <key>Path</key>
    <string>/c=x240b-4zz18-cx9p60347rmsyoi</string>
    <key>Access Timestamp</key>
    <string>1618953853339</string>
    <key>Labels</key>
    <array>
    </array>
</dict>
</plist>

New version first commit: https://dev.arvados.org/projects/arvados-workbench-2/repository/revisions/ef925b80e9d4cb8d0c7e86c7bc0e642533eaf7b2
New version first commit: https://dev.arvados.org/projects/arvados-workbench-2/repository/revisions/aa6c77cc30b4762db93f2458aeaf3e2e41f17d2c
Test run: developer-tests-workbench2: #393
Test run: developer-tests-workbench2: #394

Added path for ip based hostnames

This wasn't addressed, is there a reason why?

At file src/views-components/webdav-s3-dialog/webdav-s3-dialog.tsx
Line 72: Can you explain why you split and re-join the the returned string? If you're providing the template's string on the code, it shouldn't be necessary to do that kind of "cleaning".

It is used to remove windows endlines which cause file not being parsed properly

Test run: developer-tests-workbench2: #395

Daniel Kutyła wrote:

This wasn't addressed, is there a reason why?

At file src/views-components/webdav-s3-dialog/webdav-s3-dialog.tsx
Line 72: Can you explain why you split and re-join the the returned string? If you're providing the template's string on the code, it shouldn't be necessary to do that kind of "cleaning".

It is used to remove windows endlines which cause file not being parsed properly

I'm not sure what you mean by this. I've tried removing the splitting+re-joining calls from the code and the generated file worked just fine.

• At file src/views-components/webdav-s3-dialog/webdav-s3-dialog.tsx Line 48 there's some console logging that I think you didn't meant to leave.
• At file cypress/integration/collection.spec.js Line 43 there's a cy.doSearch(collectionUUID) call that can be replaced with the recently merged cy.goToPath('/collections/collectionUUID'), it will be faster because it uses the app's router instead of doing a wb2 search.

The rest LGTM.