Arvados

Review comments for branch: 3894-gem-version

Tom, looks good. I spent some time trying to understand what magic the " . " is doing in this update. Thanks to your very detailed git log comment that clarified the intent.

A couple more changes pushed, now at c446dd8

Reviewing arvados-dev 07b0b88

• A default value for CONFIGSRC is set under if [[ -n "$CONFIGSRC" ]]. Should that check -z instead? Otherwise, it seems like you only override any custom value. You could also flatten this condition with the -d check below it.
  • It would be cool if the script set a default CONFIGSRC early on, then let me override it later. Then I could run it locally with CONFIGSRC= to use the configuration files I already have, without organizing copies. The test in install_apiserver makes me think you had this in mind too.
• The handling of Python egg_info is inconsistent: the SDK runs it through a specific cut, while FUSE does not. The cut makes it look like you thought about switching the format to use %ci instead of %ct, but then backed that out without reverting the cut. Which is it? (My personal opinion is that we should switch to git log --first-parent --format=format:%ci.%h -n1 . | tr -dc 0-9a-f.. I believe this would be safe, and promote consistency with our Gem versioning. But the only thing required for this story is adding . and keeping them consistent.)

Thanks.

Brett Smith wrote:

Reviewing arvados-dev 07b0b88

• A default value for CONFIGSRC is set under if [[ -n "$CONFIGSRC" ]]. Should that check -z instead? Otherwise, it seems like you only override any custom value. You could also flatten this condition with the -d check below it.

Yes, meant -z, thanks. Fixed (with flatten) in 63517ba

• It would be cool if the script set a default CONFIGSRC early on, then let me override it later. Then I could run it locally with CONFIGSRC= to use the configuration files I already have, without organizing copies. The test in install_apiserver makes me think you had this in mind too.

• If you provide CONFIGSRC=/something on the run-tests.sh command line, config files get copied from there.
• Otherwise, run-tests.sh doesn't touch your config files.
• Exception: If $HOME/arvados-api-server/ is a directory, we assume you're ci.curoverse.com and use that as the default CONFIGSRC. (We should probably just update ci and drop the magic default, if the first two options seem reasonable...)

Re "Then I could run it locally..." -- you can, right? (Is this a reference to the -z mistake above, or is there another bug here I'm still not seeing?)

• The handling of Python egg_info is inconsistent: the SDK runs it through a specific cut, while FUSE does not. The cut makes it look like you thought about switching the format to use %ci instead of %ct, but then backed that out without reverting the cut. Which is it? (My personal opinion is that we should switch to git log --first-parent --format=format:%ci.%h -n1 . | tr -dc 0-9a-f.. I believe this would be safe, and promote consistency with our Gem versioning. But the only thing required for this story is adding . and keeping them consistent.)

Whoops. OK, I forgot to update FUSE. Fixed that in ee10e6b. Also made the change to %ci that I had intended, as you guessed. Used tr -dc instead of a horrible cut list (but added back a small cut to eliminate the 0400 for the time zone).

Tom Clegg wrote:

Re "Then I could run it locally..." -- you can, right? (Is this a reference to the -z mistake above, or is there another bug here I'm still not seeing?)

I sort of missed the purpose of the later -d check. What you describe seems sensible and works for me. Please go ahead and merge this. Thanks.