Project

General

Profile

Actions

Feature #4465

closed

[Workbench] Link "apiserver version" to a redmine or github revision history page

Added by Tom Clegg over 9 years ago. Updated over 9 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Tim Pierce
Category:
-
Target version:
Story points:
0.5

Description

Link "Workbench version" to:

https://github.com/curoverse/arvados/tree/{commit}/apps/workbench

Link "API version" to:

https://github.com/curoverse/arvados/tree/{commit}/services/api

Subtasks 2 (0 open2 closed)

Task #4709: Review 4465-workbench-version-linksResolvedTim Pierce12/02/2014Actions
Task #4614: add apiserver version linkResolvedTim Pierce12/02/2014Actions

Related issues

Related to Arvados - Bug #4175: [API] API method and interface to check if a bugfix is deployed on a clusterNewActions
Actions #1

Updated by Tom Clegg over 9 years ago

  • Target version changed from Arvados Future Sprints to 2014-12-10 sprint
Actions #2

Updated by Tim Pierce over 9 years ago

  • Assigned To set to Tim Pierce
Actions #3

Updated by Tim Pierce over 9 years ago

  • Description updated (diff)
Actions #4

Updated by Tim Pierce over 9 years ago

  • Status changed from New to In Progress
Actions #5

Updated by Brett Smith over 9 years ago

At 3374f67, I have one bug report and one test comment.

The bug report: the API server version reported in the discovery document can include a "locally modified" string, just like the Workbench version. When it does, the current code generates a bad GitHub link. You might fix it by having separate api_version and api_version_text methods as with Workbench; the version method can return what's in version_text until the first non-hex character. But that's just one suggestion; I'm sure there are other options.

Which brings me to my test comment: because this story is very specific about where the links should go, you could make the tests stronger by checking that the links actually go where we want. Something like:

wb_version_href = "https://github.com/curoverse/aravados/#{Rails.configuration.source_version}/apps/workbench" 
refute_empty(all("a").select { |a| a[:href] == wb_version_href })
refute_empty(all("a").select { |a| a[:href] =~ /https:\/\/github.com\/curoverse\/aravados\/[0-9a-f]+\/services\/api\// })

This would also improve test isolation (though it's still not perfect) by eliminating the dependency on VersionHelper. Those could be tested independently in dedicated helper tests. Those would be nice and easy to write because you can just set the configuration values to whatever you need, then check the string results directly.

Thanks.

Actions #6

Updated by Tim Pierce over 9 years ago

Brett Smith wrote:

At 3374f67, I have one bug report and one test comment.

The bug report: the API server version reported in the discovery document can include a "locally modified" string, just like the Workbench version. When it does, the current code generates a bad GitHub link. You might fix it by having separate api_version and api_version_text methods as with Workbench; the version method can return what's in version_text until the first non-hex character. But that's just one suggestion; I'm sure there are other options.

Good call, and I'm fine with that suggestion. Added at b629c24.

Which brings me to my test comment: because this story is very specific about where the links should go, you could make the tests stronger by checking that the links actually go where we want. Something like:

[...]

As background: this story is only specific about where the links should go because, uh, I wrote that specification to describe what I was doing. When I inherited the story it had no description at all, and I read #4175 to get more context about what we wanted it to do here. The github links that I added were my best guess as to what would be useful for someone who needs to investigate a version-related bug issue.

(I didn't mean to slip one over anybody, just keep velocity high -- I intended to ask explicitly whether this is the right choice when I requested the code review, I just forgot.)

That said, I do agree that it makes more sense in the end for the integration test not to be dependent on the VersionHelper. Updated tests at b0001b6 to look for link targets that refer to Github links into the repository history.

Actions #7

Updated by Brett Smith over 9 years ago

Tim Pierce wrote:

As background: this story is only specific about where the links should go because, uh, I wrote that specification to describe what I was doing. When I inherited the story it had no description at all, and I read #4175 to get more context about what we wanted it to do here. The github links that I added were my best guess as to what would be useful for someone who needs to investigate a version-related bug issue.

Thanks for pointing that out. In fairness, the subject at least mentions a GitHub link, so I think you're pretty well in the clear here.

My one nit for b0001b6d is that it might be nice to end the text regexps with /?, so tests don't start failing if a trailing slash starts getting added somewhere. This looks good to merge, thanks.

Actions #8

Updated by Tim Pierce over 9 years ago

  • Status changed from In Progress to Resolved
  • % Done changed from 50 to 100

Applied in changeset arvados|commit:86f994bb5d039e62bfc4383ed9f510b77b71ecad.

Actions

Also available in: Atom PDF