Arvados

Ready for review @commit:de1f02c44bf8da29b1c5194efc29daf781b750bc

Don't do the "options ...bool" thing in the test suite. Just add a loadKeepstoresFromConfig bool arg, and add ,true or ,false to the existing runProxy() uses.

It looks like the additional ServerRequiredConfigYmlSuite ended up being unnecessary, and the new TestGetIndex could be (*ServerRequiredSuite)TestGetIndexUsingConfigYAML()... or is there a subtle reason why the separate suite is needed? (Need to restart servers in between? Need more than 2 keepstores?)

You can avoid the extra "associated revisions" in redmine (which don't go away when you amend/rebase) by tagging commits "16328: blah..." and leaving out the "refs #16328" bit until the merge-to-master commit.

Nit: Even though identifiers in _test.go don't get exported anyway, I'd use private lowercase getIndexWorker instead of GetIndexWorker.

The actual code change LGTM.

Tom Clegg wrote:

Don't do the "options ...bool" thing in the test suite. Just add a loadKeepstoresFromConfig bool arg, and add ,true or ,false to the existing runProxy() uses.

OK. I've simplified this further and removed the additional argument to runProxy. I've replaced it with a variable that is set in SetUpSuite for ServerRequiredConfigYmlSuite.

It looks like the additional ServerRequiredConfigYmlSuite ended up being unnecessary, and the new TestGetIndex could be (*ServerRequiredSuite)TestGetIndexUsingConfigYAML()... or is there a subtle reason why the separate suite is needed? (Need to restart servers in between? Need more than 2 keepstores?)

Yeah, the config.yml defines 4 keepstores, so I need them all to be up (or the index test will fail). I've put a comment to that effect in the SetUpSuite for ServerRequiredConfigYmlSuite.

You can avoid the extra "associated revisions" in redmine (which don't go away when you amend/rebase) by tagging commits "16328: blah..." and leaving out the "refs #16328" bit until the merge-to-master commit.

Ah, yes. Will do going forward.

Nit: Even though identifiers in _test.go don't get exported anyway, I'd use private lowercase getIndexWorker instead of GetIndexWorker.

Sure, changed.

The actual code change LGTM.

Thanks, updated changes at a77a856600ef5d22208eff74d909ff6d1dc4664d

OK, I reverted the simplification after talking to Tom and added the paramter to runProxy, and merged. Thanks!