Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use deployment-status ref instead of sha #915

Merged
merged 3 commits into from
Aug 21, 2019

Conversation

dennissivia
Copy link
Contributor

@dennissivia dennissivia commented Aug 20, 2019

Summary

When the integrator that created the deployment status provided a ref,
that is different from the commit sha, that ref is now displayed instead
of the short sha.
This addresses the issue discussed in #98 and #85.
I think the next step would be doing an actual API request to fetch the
branches, but his change is a 0-cost improvement and (almost) 0-effort
improvement that allows us to gather metrics about custom ref adoption
and maybe this already solves the problem.

Screenshots

Before

depoyment-status-before

After

deployment-status-after

Closes #85, #98

@dennissivia dennissivia requested a review from a team August 20, 2019 13:59
@codecov
Copy link

codecov bot commented Aug 20, 2019

Codecov Report

Merging #915 into cou929-subscribe-certain-labels will increase coverage by <.01%.
The diff coverage is 100%.

@@                         Coverage Diff                         @@
##           cou929-subscribe-certain-labels     #915      +/-   ##
===================================================================
+ Coverage                            97.21%   97.22%   +<.01%     
===================================================================
  Files                                  157      157              
  Lines                                 2623     2627       +4     
  Branches                               356      358       +2     
===================================================================
+ Hits                                  2550     2554       +4     
  Misses                                  69       69              
  Partials                                 4        4

@codecov
Copy link

codecov bot commented Aug 20, 2019

Codecov Report

Merging #915 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #915      +/-   ##
==========================================
+ Coverage   97.33%   97.34%   +<.01%     
==========================================
  Files         154      154              
  Lines        2481     2487       +6     
  Branches      321      323       +2     
==========================================
+ Hits         2415     2421       +6     
  Misses         62       62              
  Partials        4        4

@dennissivia dennissivia marked this pull request as ready for review August 20, 2019 14:51
@dennissivia dennissivia added topic:deployments Deployment and Deployment-Status related status:Do not merge Do not merge (quite yet) for some reason labels Aug 20, 2019
Copy link
Contributor

@wilhelmklopp wilhelmklopp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an awesome improvement! 🎉 🌈

When the integrator that created the deployment status provided a ref,
that is different from the commit sha, that ref is now displayed instead
of the short sha.

Do you have an example screenshot of what this looks like in Slack?

lib/activity/deployments.js Outdated Show resolved Hide resolved
lib/messages/deployment-status.js Outdated Show resolved Hide resolved
lib/messages/deployment-status.js Outdated Show resolved Hide resolved
lib/messages/deployment-status.js Outdated Show resolved Hide resolved
lib/messages/deployment-status.js Outdated Show resolved Hide resolved
When the integrator that created the deployment status provided a ref,
that is different from the commit sha, that ref is now displayed instead
of the short sha.
This addresses the issue discussed in #98 and #85.
I think the next step would be doing an actual API request to fetch the
branches, but his change is a 0-cost improvement and (almost) 0-effort
improvement that allows us to gather metrics about custom ref adoption
and maybe this already solves the problem.

Refs #85, #98
Most importantly we are now showing showing the ref and the shortSha and
not only the ref to improve the UX for certain use-cases.
@dennissivia dennissivia force-pushed the improve-deployment-message branch from b27de00 to f87d8ea Compare August 21, 2019 11:55
@dennissivia dennissivia changed the base branch from cou929-subscribe-certain-labels to master August 21, 2019 11:56
@dennissivia dennissivia removed the status:Do not merge Do not merge (quite yet) for some reason label Aug 21, 2019
@dennissivia dennissivia merged commit 9c3c295 into master Aug 21, 2019
@dennissivia dennissivia deleted the improve-deployment-message branch August 21, 2019 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic:deployments Deployment and Deployment-Status related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

show ref instead of sha if one exists
2 participants