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

ENH: Use check for PR instead of status #73

Merged
merged 3 commits into from
Sep 3, 2019

Conversation

pllim
Copy link
Contributor

@pllim pllim commented May 1, 2019

Follow up of #45 to actually use the set_check API.

TODO:

  • Is this what we really want?
  • Fix tests, if they fail.
  • How to play with this before merge for all the scenarios being handled in the PR checks?

@pllim pllim added the enhancement New feature or request label May 1, 2019
@pllim pllim requested review from astrofrog and Cadair May 1, 2019 04:51
@astrofrog
Copy link
Collaborator

I think we do want this, because the text is sometimes too long for the status bar. I'm not sure if you can play with it for all the scenarios but I'd definitely recommend trying it out on a test repository just to get a sense for what this will look like. If you set it up for a test repo, I can try and open some pull requests to see how it behaves :)

@bsipocz
Copy link
Member

bsipocz commented Jun 19, 2019

What would this mean in practice besides adding the "checks" tab on the PR? Would the way how the statuses currently are reported back change, so one always need to click to go to the "checks" tab?

@astrofrog
Copy link
Collaborator

I'm not 100% sure, so it would be nice to see this in action in a test repo

@pllim
Copy link
Contributor Author

pllim commented Jun 19, 2019

@astrofrog , pretty sure we tested this in Tucson last year. You were sitting next to me. And we even hi-fived.

@Cadair
Copy link
Member

Cadair commented Jun 19, 2019

I would imagine it would work like Azure does.

I am onboard with this plan, given we already removed the auto-comment builder functionality 😛

@astrofrog
Copy link
Collaborator

Oh yeah I do remember testing it now :) We might have to test it again to address some of @Cadair's comments though

@pllim
Copy link
Contributor Author

pllim commented Aug 5, 2019

@Cadair 's comment addressed. How do I test this on a test repo? @astrofrog , I remember you doing some magic with the experimental branch, is that the way to go?

@pllim pllim changed the title [WIP] Use check for PR instead of status ENH: Use check for PR instead of status Aug 6, 2019
@@ -62,7 +62,7 @@ def invalidate_cache(self):
@property
def _headers(self):
if self.installation is None:
return None
return {}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need this change to test stuff without installation number.

@codecov-io
Copy link

Codecov Report

Merging #73 into master will decrease coverage by 0.99%.
The diff coverage is 82.6%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #73   +/-   ##
=======================================
- Coverage   82.33%   81.34%   -1%     
=======================================
  Files          16       16           
  Lines         804      820   +16     
=======================================
+ Hits          662      667    +5     
- Misses        142      153   +11
Impacted Files Coverage Δ
baldrick/plugins/github_pull_requests.py 76.11% <66.66%> (ø) ⬆️
baldrick/github/github_api.py 63.22% <88.23%> (-1.64%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ba92ebb...033df7c. Read the comment docs.

@pllim
Copy link
Contributor Author

pllim commented Aug 6, 2019

Update: I finally got around to fixing the tests. I think this is ready for a test run on the "experimental" branch.

Err... wait, where is the experimental bot?

@Cadair
Copy link
Member

Cadair commented Aug 23, 2019

I just deployed this on my test giles instance: Cadair/testgiles#2

@pllim
Copy link
Contributor Author

pllim commented Aug 23, 2019

How you liking it?

@Cadair
Copy link
Member

Cadair commented Aug 23, 2019

It seems to work, I just pushed it out to Giles proper

Copy link
Collaborator

@astrofrog astrofrog left a comment

Choose a reason for hiding this comment

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

Looks good to me. To test this out once merged, we need to push an update to the experimental branch of astropy-bot (after making sure it's up to date with master) and the experimental bot will then be active on the following repository:

https://github.com/astropy/astropy-bot-sandbox

@Cadair Cadair merged commit f67c60f into OpenAstronomy:master Sep 3, 2019
@pllim pllim deleted the use-set-check branch September 3, 2019 12:59
@pllim
Copy link
Contributor Author

pllim commented Sep 3, 2019

@astrofrog , I made astropy-bot experimental branch in sync with master. Then I opened astropy/astropy-bot-sandbox#1 but no GitHub Checks appear. How long does it take for the experimental branch to deploy? More than a few secs?

@Cadair Cadair mentioned this pull request Sep 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants