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

Adopt Github Deployments #26

Merged
merged 10 commits into from
Jun 6, 2017
Merged

Adopt Github Deployments #26

merged 10 commits into from
Jun 6, 2017

Conversation

paulirish
Copy link
Contributor

@paulirish paulirish commented Jun 3, 2017

fixes #17

This was pretty straightforward. Here's an example PR where I was using this: paulirish/hello-world#3 (comment)

Pending State:
image

Completed:
image

And I definitely prefer this setup based on deployments rather than commit statuses. It's very visible, whereas once the commit status's all pass, that gets hidden by Github.

Configurability:

  • the staging environment word in both states is set by us. (in the 1st screenshot i used "staging" and in the 2nd its "staging on now.sh".)
  • there's tooltip text set on the "deployed" link in the Completed state that we can configure
  • Interestingly, there's a description field in for 1) creating a deployment and 2) updating a pending deployment, but it doesn't appear to affect the user-visible UI. shrug

cc @mxstbr


As a driveby I changed the alias from branch ref to PR number. I can see the argument for keeping things as they were, so no big deal if you guys want to revert that. :)

@mxstbr
Copy link

mxstbr commented Jun 3, 2017

YES this is awesome @paulirish! Can't wait to upgrade 🙌

@creichert
Copy link

I think this looks really good. Only thing I would add is possibly adding a second value of application/json to the Accept header behind the ant-man preview. I'm not really sure how github handles that once they've made it a mainline feature (hopefully soon) but it might be prudent.

Either way, awesome work!

@paulirish
Copy link
Contributor Author

Only thing I would add is possibly adding a second value of application/json to the Accept header behind the ant-man preview.

Works for me. Done.

Also added a fix that means pull requests based on branches on other remotes can work now. Beforehand the branch clone failed because it wasn't local to origin (more or less). Now, we clone the pull request's repo and build that.

@zpnk
Copy link
Owner

zpnk commented Jun 6, 2017

Thanks for this @paulirish, nice work! Really like how the deployments look in the GH ui, much more visible/usable than commit statuses.

Good catch on the remote branch issue, glad to get that fix in there. 👍

On the alias name - I think I prefer the branch name over a PR number but I'm definitely open to feedback/thoughts on that. Could go either way tbh. Perhaps one day this could be configurable (somehow 😛 ).

I'll be testing this out a little bit, but it should be good to merge!

@paulirish
Copy link
Contributor Author

I think I prefer the branch name over a PR number but I'm definitely open to feedback/thoughts on that.

sgtm. reverted that for now.

cheers. thanks for taking a look.

@zpnk
Copy link
Owner

zpnk commented Jun 6, 2017

@paulirish Everything looks great, I'm going to merge this in. Really appreciate your work on this feature! 👏

@zpnk zpnk merged commit f4cf708 into zpnk:master Jun 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GitHub deployment events?
4 participants