Bug 148387 - Add support to dashboard for displaying Git SHA's as revisions.
Summary: Add support to dashboard for displaying Git SHA's as revisions.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-08-24 12:27 PDT by Jason Marcell
Modified: 2015-08-24 15:43 PDT (History)
9 users (show)

See Also:


Attachments
Patch (3.47 KB, patch)
2015-08-24 12:29 PDT, Jason Marcell
no flags Details | Formatted Diff | Diff
Patch (4.63 KB, patch)
2015-08-24 14:37 PDT, Jason Marcell
no flags Details | Formatted Diff | Diff
Patch (4.64 KB, patch)
2015-08-24 14:44 PDT, Jason Marcell
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jason Marcell 2015-08-24 12:27:15 PDT
Add support to dashboard for displaying Git SHA's as revisions.
Comment 1 Jason Marcell 2015-08-24 12:29:04 PDT
Created attachment 259764 [details]
Patch
Comment 2 Jason Marcell 2015-08-24 12:31:15 PDT
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 3 Daniel Bates 2015-08-24 13:28:05 PDT
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.
Comment 4 Jason Marcell 2015-08-24 14:37:13 PDT
Created attachment 259774 [details]
Patch
Comment 5 Jason Marcell 2015-08-24 14:38:46 PDT
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 6 Daniel Bates 2015-08-24 14:41:47 PDT
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.
Comment 7 Jason Marcell 2015-08-24 14:44:55 PDT
Created attachment 259779 [details]
Patch
Comment 8 WebKit Commit Bot 2015-08-24 15:43:08 PDT
Comment on attachment 259779 [details]
Patch

Clearing flags on attachment: 259779

Committed r188890: <http://trac.webkit.org/changeset/188890>
Comment 9 WebKit Commit Bot 2015-08-24 15:43:13 PDT
All reviewed patches have been landed.  Closing bug.