Feature #4465

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

Added by Tom Clegg about 6 years ago. Updated almost 6 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Tim Pierce
Category:
-
Target version:
Start date:
12/02/2014
Due date:
% Done:

100%

Estimated time:
(Total: 0.50 h)
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

Task #4709: Review 4465-workbench-version-linksResolvedTim Pierce

Task #4614: add apiserver version linkResolvedTim Pierce


Related issues

Related to Arvados - Bug #4175: [API] API method and interface to check if a bugfix is deployed on a clusterNew10/09/2014

Associated revisions

Revision 86f994bb
Added by Tim Pierce almost 6 years ago

Merge branch '4465-workbench-version-links'

Fixes #4465.

Revision 3f904a5e (diff)
Added by Tom Clegg almost 6 years ago

4465: Link to arvados.org revision history instead of github.com code view.

Also, remove spurious helper inclusions, and fix 'locally modified'
detection (it was broken because "" is truthy).

refs #4465

History

#1 Updated by Tom Clegg about 6 years ago

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

#2 Updated by Tim Pierce about 6 years ago

  • Assigned To set to Tim Pierce

#3 Updated by Tim Pierce almost 6 years ago

  • Description updated (diff)

#4 Updated by Tim Pierce almost 6 years ago

  • Status changed from New to In Progress

#5 Updated by Brett Smith almost 6 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.

#6 Updated by Tim Pierce almost 6 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.

#7 Updated by Brett Smith almost 6 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.

#8 Updated by Tim Pierce almost 6 years ago

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

Applied in changeset arvados|commit:86f994bb5d039e62bfc4383ed9f510b77b71ecad.

Also available in: Atom PDF