Summary: | Add support to dashboard for displaying Git SHA's as revisions. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jason Marcell <jmarcell> | ||||||||
Component: | Tools / Tests | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | adele, ap, bburg, commit-queue, dbates, ddkilzer, dean_johnson, jake.nielsen.webkit, jmarcell | ||||||||
Priority: | P2 | ||||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Jason Marcell
2015-08-24 12:27:15 PDT
Created attachment 259764 [details]
Patch
Comment on attachment 259764 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=259764&action=review > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:223 > + content.textContent = revision.substr(0, 7); Are we okay with truncating this to 7 characters for display? Do we need to add any commit explaining the choice to truncate or the choice of 7 characters? > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:225 > + content.textContent = revision; Should we instead throw an exception here? Or perhaps just leave the "else" clause out? Comment on attachment 259764 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=259764&action=review We should add the isSVN property to the definition of the OpenSource repository in this patch. Otherwise, we will no longer prefix OpenSource revisions with 'r' following this patch. >> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:223 >> + content.textContent = revision.substr(0, 7); > > Are we okay with truncating this to 7 characters for display? Do we need to add any commit explaining the choice to truncate or the choice of 7 characters? You could add a comment to explain that a SHA of 7 characters is sufficiently unique for our purposes. >> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:225 >> + content.textContent = revision; > > Should we instead throw an exception here? Or perhaps just leave the "else" clause out? If you are going to handle each kind of SCM then you could add a console.assert(false, "Should not get here; " + repository.name + " did not specify a known VCS type."); Alternatively, you could write this logic such that Git is the special case and SVN is the fallback case. Created attachment 259774 [details]
Patch
Comment on attachment 259774 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=259774&action=review > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Dashboard.js:41 > + Internal: { name: "internal", isSVN: true, order: 1 }, I believe we can now remove "Internal" from this file, but we could do that in a separate patch. Comment on attachment 259774 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=259774&action=review > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:225 > + else if (repository.isGit) > + // Truncating for display. Git traditionally uses seven characters for a short hash. > + content.textContent = revision.substr(0, 7); > + else Please add curly-braces. Created attachment 259779 [details]
Patch
Comment on attachment 259779 [details] Patch Clearing flags on attachment: 259779 Committed r188890: <http://trac.webkit.org/changeset/188890> All reviewed patches have been landed. Closing bug. |