Bug 148387 – Add support to dashboard for displaying Git SHA's as revisions.

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jason Marcell]

Add support to dashboard for displaying Git SHA's as revisions.

[Jason Marcell]

Created attachment 259764 [details]
Patch

[Jason Marcell]

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?

[Daniel Bates]

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.

[Jason Marcell]

Created attachment 259774 [details]
Patch

[Jason Marcell]

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.

[Daniel Bates]

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.

[Jason Marcell]

Created attachment 259779 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 259779 [details]
Patch

Clearing flags on attachment: 259779

Committed r188890: <http://trac.webkit.org/changeset/188890>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.