https://dev.arvados.org/https://dev.arvados.org/favicon.ico?15576888422021-03-26T15:45:31ZArvadosArvados - Feature #17415: Create Mountainduck Bookmark fileshttps://dev.arvados.org/issues/17415?journal_id=914612021-03-26T15:45:31ZDaniel Kutyła
<ul><li><strong>Status</strong> changed from <i>New</i> to <i>In Progress</i></li></ul> Arvados - Feature #17415: Create Mountainduck Bookmark fileshttps://dev.arvados.org/issues/17415?journal_id=916752021-04-12T08:20:07ZDaniel Kutyła
<ul></ul><p>New version first commit: <a class="external" href="https://dev.arvados.org/projects/arvados-workbench-2/repository/revisions/37f3a8d7a7ce05a15fbb9219763b46ba1c250976">https://dev.arvados.org/projects/arvados-workbench-2/repository/revisions/37f3a8d7a7ce05a15fbb9219763b46ba1c250976</a><br />Test run: <a class="external" href="https://ci.arvados.org/job/developer-tests-workbench2/366/"<a href="https://ci.arvados.org/job/developer-tests-workbench2/366/">developer-tests-workbench2: #366 <img src="https://ci.arvados.org/buildStatus/icon?job=developer-tests-workbench2&build=366" alt="" /></a></a></p>
<p>Created mountainduck bookmark files generator</p> Arvados - Feature #17415: Create Mountainduck Bookmark fileshttps://dev.arvados.org/issues/17415?journal_id=916772021-04-12T15:20:48ZLucas Di Pentimalucas.dipentima@curii.com
<ul></ul><p>Reviewing <a class="changeset" title="17415: Created mountainduck bookmark files generator Arvados-DCO-1.1-Signed-off-by: Daniel Kutył..." href="https://dev.arvados.org/projects/arvados/repository/arvados-workbench2/revisions/37f3a8d7a7ce05a15fbb9219763b46ba1c250976">arvados-workbench2|37f3a8d</a></p>
<ul>
<li>As you can see on <a class="external" href="https://ci.arvados.org/job/developer-tests-workbench2/366/console"<a href="https://ci.arvados.org/job/developer-tests-workbench2/366/">developer-tests-workbench2: #366 <img src="https://ci.arvados.org/buildStatus/icon?job=developer-tests-workbench2&build=366" alt="" /></a>/console</a> -- there're some problems with the cypress test environment.</li>
<li>On the Cypress test, could we check that the downloaded <code>.duck</code> file's contents is correct instead of just checking that its size is greater than 50?</li>
<li>At file <code>src/store/collections/collection-info-actions.ts</code>
<ul>
<li>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:
<ul>
<li>Create a 'Collection A' collection inside the home project.</li>
<li>Create a 'Collection B' collection inside the home project.</li>
<li>From within the home project, click on the 'Collection A' collection.</li>
<li>Go back to the home project.</li>
<li>Right-click on the 'Collection B' collection, ask for the WebDav/S3 dialog and download the mountain duck file.</li>
<li>The downloaded file will have the 'Collection A' name.</li>
</ul>
</li>
<li>Also line 38: I don't think overriding the typing system by casting to <code>any</code> is convenient.</li>
</ul>
</li>
<li>At file <code>src/views-components/webdav-s3-dialog/webdav-s3-dialog.tsx</code>
<ul>
<li>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".</li>
<li>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?</li>
</ul>
</li>
<li>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: <code>DNS lookup for davs://user@IP_or_hostname failed...</code>. Some possible problems are:
<ul>
<li>Username, protocol and port number get included on the 'hostname' field.</li>
<li>Port number is hardcoded to 443 (it can be different, for example arvbox uses a different one)</li>
<li>Password (token) doesn't get included on the file.</li>
</ul></li>
</ul> Arvados - Feature #17415: Create Mountainduck Bookmark fileshttps://dev.arvados.org/issues/17415?journal_id=916862021-04-12T15:49:26ZLucas Di Pentimalucas.dipentima@curii.com
<ul></ul><p>This is the <code>.duck</code> file that I got generated with CyberDuck Mac version 8.7.5:</p>
<pre>
<?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><a href="https://arvadosapi.com/ce8i5-4zz18-mx9loldsvg238gk">ce8i5-4zz18-mx9loldsvg238gk</a>.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>
</pre>
<p>Note that the <code>Hostname</code> key is populated with just the collection's hostname, and not the entire <code>davs://ldipenti</code>...@ URL.<br />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.</p> Arvados - Feature #17415: Create Mountainduck Bookmark fileshttps://dev.arvados.org/issues/17415?journal_id=917472021-04-13T19:51:45ZLucas Di Pentimalucas.dipentima@curii.com
<ul></ul><p>Reviewing changes made on <a class="changeset" title="17415: Fixed wrong hostname in generated duck file Arvados-DCO-1.1-Signed-off-by: Daniel Kutyła ..." href="https://dev.arvados.org/projects/arvados/repository/arvados-workbench2/revisions/c26242ac77e2acb8e4c38780c9340955a8e9daf5">arvados-workbench2|c26242a</a></p>
<p>Although changes in <code>.duck</code> 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 <a class="external" href="https://collection-uuid.hostname:9004">https://collection-uuid.hostname:9004</a>, the <code>.duck</code> file's hostname field will be filled with "collection-uuid.hostname:9004" as a hostname, producing a DNS resolution error.</p> Arvados - Feature #17415: Create Mountainduck Bookmark fileshttps://dev.arvados.org/issues/17415?journal_id=919072021-04-15T21:57:39ZDaniel Kutyła
<ul></ul><p>New version first commit: <a class="external" href="https://dev.arvados.org/projects/arvados-workbench-2/repository/revisions/645ea9356ef315154681d093cd6a42c1a8984e12">https://dev.arvados.org/projects/arvados-workbench-2/repository/revisions/645ea9356ef315154681d093cd6a42c1a8984e12</a><br />Test run: <a class="external" href="https://ci.arvados.org/job/developer-tests-workbench2/371/"<a href="https://ci.arvados.org/job/developer-tests-workbench2/371/">developer-tests-workbench2: #371 <img src="https://ci.arvados.org/buildStatus/icon?job=developer-tests-workbench2&build=371" alt="" /></a></a></p>
<p>Fixed duck file added better tests</p> Arvados - Feature #17415: Create Mountainduck Bookmark fileshttps://dev.arvados.org/issues/17415?journal_id=919152021-04-16T14:39:26ZLucas Di Pentimalucas.dipentima@curii.com
<ul></ul><p>Tests are failing the same way as described on <a href="#note-3">#note-3</a>: <a class="external" href="https://ci.arvados.org/job/developer-tests-workbench2/371/console"<a href="https://ci.arvados.org/job/developer-tests-workbench2/371/">developer-tests-workbench2: #371 <img src="https://ci.arvados.org/buildStatus/icon?job=developer-tests-workbench2&build=371" alt="" /></a>/console</a></p> Arvados - Feature #17415: Create Mountainduck Bookmark fileshttps://dev.arvados.org/issues/17415?journal_id=920722021-04-20T19:16:38ZDaniel Kutyła
<ul></ul><p>New version first commit: <a class="external" href="https://dev.arvados.org/projects/arvados-workbench-2/repository/revisions/eaf76a93400029d3d788264b829bf878aad1f661">https://dev.arvados.org/projects/arvados-workbench-2/repository/revisions/eaf76a93400029d3d788264b829bf878aad1f661</a><br />Test run: <a class="external" href="https://ci.arvados.org/job/developer-tests-workbench2/381/"<a href="https://ci.arvados.org/job/developer-tests-workbench2/381/">developer-tests-workbench2: #381 <img src="https://ci.arvados.org/buildStatus/icon?job=developer-tests-workbench2&build=381" alt="" /></a></a></p>
<p>Cypress upgrade to 3.6 in order to fix electron download issue</p> Arvados - Feature #17415: Create Mountainduck Bookmark fileshttps://dev.arvados.org/issues/17415?journal_id=920752021-04-20T19:27:39ZLucas Di Pentimalucas.dipentima@curii.com
<ul></ul><p>Please remove the <code>it.only()</code> call and re-run tests.</p> Arvados - Feature #17415: Create Mountainduck Bookmark fileshttps://dev.arvados.org/issues/17415?journal_id=920762021-04-20T19:31:45ZDaniel Kutyła
<ul></ul><p>New version first commit: <a class="external" href="https://dev.arvados.org/projects/arvados-workbench-2/repository/revisions/0569677b7c4d4a907950b6dfc6218eabaa2aad59">https://dev.arvados.org/projects/arvados-workbench-2/repository/revisions/0569677b7c4d4a907950b6dfc6218eabaa2aad59</a><br />Test run: <a class="external" href="https://ci.arvados.org/job/developer-tests-workbench2/384/"<a href="https://ci.arvados.org/job/developer-tests-workbench2/384/">developer-tests-workbench2: #384 <img src="https://ci.arvados.org/buildStatus/icon?job=developer-tests-workbench2&build=384" alt="" /></a></a></p>
<p>Removed only</p> Arvados - Feature #17415: Create Mountainduck Bookmark fileshttps://dev.arvados.org/issues/17415?journal_id=920802021-04-20T21:32:48ZLucas Di Pentimalucas.dipentima@curii.com
<ul></ul><p>Reviewing <a class="changeset" title="17415: Removed only Arvados-DCO-1.1-Signed-off-by: Daniel Kutyła <daniel.kutyla@contractors.roch..." href="https://dev.arvados.org/projects/arvados/repository/arvados-workbench2/revisions/0569677b7c4d4a907950b6dfc6218eabaa2aad59">arvados-workbench2|0569677b</a></p>
<p>Lucas Di Pentima wrote:</p>
<blockquote>
<p>Reviewing <a class="changeset" title="17415: Created mountainduck bookmark files generator Arvados-DCO-1.1-Signed-off-by: Daniel Kutył..." href="https://dev.arvados.org/projects/arvados/repository/arvados-workbench2/revisions/37f3a8d7a7ce05a15fbb9219763b46ba1c250976">arvados-workbench2|37f3a8d</a></p>
<ul>
<li>At file <code>src/store/collections/collection-info-actions.ts</code>
<ul>
<li>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:
<ul>
<li>Create a 'Collection A' collection inside the home project.</li>
<li>Create a 'Collection B' collection inside the home project.</li>
<li>From within the home project, click on the 'Collection A' collection.</li>
<li>Go back to the home project.</li>
<li>Right-click on the 'Collection B' collection, ask for the WebDav/S3 dialog and download the mountain duck file.</li>
<li>The downloaded file will have the 'Collection A' name.</li>
</ul></li>
</ul></li>
</ul>
</blockquote>
<p>The above issue keeps happening.</p>
<blockquote>
<ul>
<li>Also line 38: I don't think overriding the typing system by casting to <code>any</code> is convenient.</li>
</ul>
</blockquote>
<p>This wasn't addressed, is there a reason why?</p>
<blockquote>
<ul>
<li>At file <code>src/views-components/webdav-s3-dialog/webdav-s3-dialog.tsx</code>
<ul>
<li>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".</li>
</ul></li>
</ul>
</blockquote>
<p>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?</p>
<blockquote>
<ul>
<li>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?</li>
</ul>
</blockquote>
<p>The button still states 'download config' on its label, do you think that's a clear enough name?</p>
<ul>
<li>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.</li>
<li>I think you should commit the updated <code>yarn.lock</code> file.</li>
<li>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 <code>arvbox</code> use case), the bookmark file should include a <code>Path</code> key with the value <code>/c=<collection uuid or pdh></code> as described on <a class="external" href="https://doc.arvados.org/v2.1/api/keep-webdav.html">https://doc.arvados.org/v2.1/api/keep-webdav.html</a></li>
</ul>
<p>Here's an example that works on my arvbox instance:</p>
<pre>
<?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><a href="https://arvadosapi.com/x240b-4zz18-cx9p60347rmsyoi">x240b-4zz18-cx9p60347rmsyoi</a></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>
</pre> Arvados - Feature #17415: Create Mountainduck Bookmark fileshttps://dev.arvados.org/issues/17415?journal_id=921392021-04-23T11:06:46ZDaniel Kutyła
<ul></ul><p>New version first commit: <a class="external" href="https://dev.arvados.org/projects/arvados-workbench-2/repository/revisions/ef925b80e9d4cb8d0c7e86c7bc0e642533eaf7b2">https://dev.arvados.org/projects/arvados-workbench-2/repository/revisions/ef925b80e9d4cb8d0c7e86c7bc0e642533eaf7b2</a><br />New version first commit: <a class="external" href="https://dev.arvados.org/projects/arvados-workbench-2/repository/revisions/aa6c77cc30b4762db93f2458aeaf3e2e41f17d2c">https://dev.arvados.org/projects/arvados-workbench-2/repository/revisions/aa6c77cc30b4762db93f2458aeaf3e2e41f17d2c</a><br />Test run: <a class="external" href="https://ci.arvados.org/job/developer-tests-workbench2/393/"<a href="https://ci.arvados.org/job/developer-tests-workbench2/393/">developer-tests-workbench2: #393 <img src="https://ci.arvados.org/buildStatus/icon?job=developer-tests-workbench2&build=393" alt="" /></a></a><br />Test run: <a class="external" href="https://ci.arvados.org/job/developer-tests-workbench2/394/"<a href="https://ci.arvados.org/job/developer-tests-workbench2/394/">developer-tests-workbench2: #394 <img src="https://ci.arvados.org/buildStatus/icon?job=developer-tests-workbench2&build=394" alt="" /></a></a></p>
<p>Added path for ip based hostnames</p> Arvados - Feature #17415: Create Mountainduck Bookmark fileshttps://dev.arvados.org/issues/17415?journal_id=921402021-04-23T11:07:50ZDaniel Kutyła
<ul></ul><p>This wasn't addressed, is there a reason why?</p>
<p>At file src/views-components/webdav-s3-dialog/webdav-s3-dialog.tsx<br />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".</p>
<p>It is used to remove windows endlines which cause file not being parsed properly</p> Arvados - Feature #17415: Create Mountainduck Bookmark fileshttps://dev.arvados.org/issues/17415?journal_id=921422021-04-23T13:50:25ZDaniel Kutyła
<ul></ul><p>Test run: <a class="external" href="https://ci.arvados.org/job/developer-tests-workbench2/395/"<a href="https://ci.arvados.org/job/developer-tests-workbench2/395/">developer-tests-workbench2: #395 <img src="https://ci.arvados.org/buildStatus/icon?job=developer-tests-workbench2&build=395" alt="" /></a></a></p> Arvados - Feature #17415: Create Mountainduck Bookmark fileshttps://dev.arvados.org/issues/17415?journal_id=921512021-04-23T18:24:59ZLucas Di Pentimalucas.dipentima@curii.com
<ul></ul><p>Daniel Kutyła wrote:</p>
<blockquote>
<p>This wasn't addressed, is there a reason why?</p>
<p>At file src/views-components/webdav-s3-dialog/webdav-s3-dialog.tsx<br />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".</p>
<p>It is used to remove windows endlines which cause file not being parsed properly</p>
</blockquote>
<p>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.</p>
Just a couple more comments:
<ul>
<li>At file <code>src/views-components/webdav-s3-dialog/webdav-s3-dialog.tsx</code> Line 48 there's some console logging that I think you didn't meant to leave.</li>
<li>At file <code>cypress/integration/collection.spec.js</code> Line 43 there's a <code>cy.doSearch(collectionUUID)</code> call that can be replaced with the recently merged <code>cy.goToPath('/collections/collectionUUID')</code>, it will be faster because it uses the app's router instead of doing a wb2 search.</li>
</ul>
<p>The rest LGTM.</p> Arvados - Feature #17415: Create Mountainduck Bookmark fileshttps://dev.arvados.org/issues/17415?journal_id=921712021-04-26T13:41:03ZDaniel Kutyła
<ul><li><strong>% Done</strong> changed from <i>0</i> to <i>100</i></li><li><strong>Status</strong> changed from <i>In Progress</i> to <i>Resolved</i></li></ul><p>Applied in changeset <a class="changeset" title="Merge branch '17415-Mountainduck-Bookmark-files' closes #17415 Arvados-DCO-1.1-Signed-off-by: Da..." href="https://dev.arvados.org/projects/arvados/repository/arvados-workbench2/revisions/409e53a945f68678eb37dd09c8dc2ad6343d236a">arvados-workbench2|409e53a945f68678eb37dd09c8dc2ad6343d236a</a>.</p> Arvados - Feature #17415: Create Mountainduck Bookmark fileshttps://dev.arvados.org/issues/17415?journal_id=926822021-05-13T15:24:09ZPeter Amstutzpeter.amstutz@curii.com
<ul><li><strong>Release</strong> set to <i>38</i></li></ul>