Arvados

Updates at cd698336f - branch 11167-wb-remove-arvget
Test run: https://ci.curoverse.com/job/developer-run-tests/391/

• Removed arv-get related code
• Adjusted/removed tests
• Added code to refuse start when keep-web isn't configured

Pending: update documentation

Some new updates at c7187ec72
Test run: https://ci.curoverse.com/job/developer-run-tests/392/

• Removed outdated comments from the code
• Corrected a test that wasn't failing when runnning them without configuring keep-web.

Fixed a failing integration test at 8f3070e16
Test run: https://ci.curoverse.com/job/developer-run-tests/393/

I didn't spend much time looking at the updates, but here are a couple initial comments.

• collections_controller -> show file method: Since workbench won’t start if keep-web is not enabled, I don’t think you need this check anymore: “if Rails.configuration.keep_web_url or Rails.configuration.keep_web_download_url”. I think you can remove this if statement and outdent the code block here?

• please revisit the integration tests that are removed and amend them to work with keep-web instead (unless a keep-web counterpart test already exists in that context). The test 'view log via keep-web redirect' in jobs_test.rb might come handy in this direction.

Thanks.

Updates at 2918ed81152a1fe8607f433e36dceddc1cb78f74
Test run: https://ci.curoverse.com/job/developer-run-tests/399/

Addressed almost all suggestions made on the previous comment. I'm still working to adjust jobs' integration test view partial job log.

Lucas, a couple more comments in the meantime:

• application.default.yml => can you add “.com” to the test config as follows?

-  keep_web_url: http://example/c=%{uuid_or_pdh}
+  keep_web_url: http://example.com/c=%{uuid_or_pdh}

• Would it be possible to refactor the following code (similar code in anonymous_access_test and jobs_test as well) into a reusable method?

+    use_keep_web_config
+
+    token = api_fixture('api_client_authorizations')['active']['api_token']
+    data = "foo\nfile\n" 
+    datablock = `echo -n #{data.shellescape} | ARVADOS_API_TOKEN=#{token.shellescape} arv-put --no-progress --raw -`.strip
+    assert $?.success?, $?
+
+    col = nil
+    use_token 'active' do
+      mtxt = ". #{datablock} 0:#{data.length}:foo\n" 
+      col = Collection.create(manifest_text: mtxt)
+    end

Updates at 57647945b08a74bab02fed6adf947eb9adb8321f
Test run: https://ci.curoverse.com/job/developer-run-tests/407/

• Addressed comments from Radhika regarding keep_web_url setting and integration test reusable method
• Did a lot of testing on why "view partial job log" integration test fails, and discovered a bug on keep-web in the process, fixed.
• The keep-web fix isn't enough because there's another bug on our testing environment, seems that the javascript code on the failing integration test doesn't get all the http response headers from keep-web, there are 2 suspects:
  • One is nginx because it's being used as a keep-web proxy when running tests.
  • Another possibility is the headless browser being used for integration tests the one losing some headers.
• The partial log view feature was tested on arvbox and worked correctly when trying it with the keep-web fix.

Next action will be follow Tom Clegg's suggestion and try using curl from within the integration test to see if the missing headers appear, because I've been trying different nginx configurations without any success.

Updates at 02a0ac50b
Test run: https://ci.curoverse.com/job/developer-run-tests/408/

Using curl from the integration tests proved that nginx wasn't being eating those expected response headers.
Finally found the cause of the failing integration test: PhantomJS 1.9.8 seems to have a bug, as reported here: https://github.com/ariya/phantomjs/issues/13200
Changing the test to use selenium fixed the issue.

IIRC this change means "include ActionController::Live" isn't needed any more in CollectionsController.

LGTM, thanks!

I've tried removing it (3224524c1a2d6790322448565cb5a16ea0bcb874) and some tests started to fail: https://ci.curoverse.com/job/developer-run-tests/410/

Reverted that change at 36b4355956d23f3b7402fba0e18318fc70e31e27 and started a new test run (https://ci.curoverse.com/job/developer-run-tests/412/) to confirm all is good with the latest changes before merging to master.