Skip to content

Conversation

mcristina422
Copy link
Contributor

BREAKING

As defined on https://developer.github.com/v3/checks/runs/#annotations-object blob_href is no longer a parameter.

Opening this as discussed in #1241 (review)

@googlebot googlebot added the cla: yes Indication that the PR author has signed a Google Contributor License Agreement. label Jul 26, 2019
Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Thanks, @mcristina422.

Travis-CI is complaining that there is a test failing. I looked into it, and it appears that there is one more blob_href references here:
https://github.com/google/go-github/blob/master/github/checks_test.go#L597
that needs to be removed.

After that is fixed, it LGTM and then we can merge after getting a second LGTM.

@gmlewis gmlewis requested a review from gauntface July 26, 2019 20:49
@codecov
Copy link

codecov bot commented Jul 27, 2019

Codecov Report

Merging #1242 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1242   +/-   ##
=======================================
  Coverage   73.42%   73.42%           
=======================================
  Files          86       86           
  Lines        6040     6040           
=======================================
  Hits         4435     4435           
  Misses        836      836           
  Partials      769      769
Impacted Files Coverage Δ
github/checks.go 62.68% <ø> (ø) ⬆️

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 9686ff0...72b0eec. Read the comment docs.

Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Thank you, @mcristina422!
LGTM.
Awaiting second LGTM before merging.

Note to self: this is a breaking API change and will require a major version bump.

@gmlewis
Copy link
Collaborator

gmlewis commented Sep 7, 2019

Thank you, @gauntface!
@willnorris - I believe this constitutes a breaking API change, therefore requiring a major semver version bump... please let me know if you disagree and then I won't bump the major version.

@willnorris
Copy link
Collaborator

@gmlewis yeah, I think you're right. But that just means the next time a new version is tagged it should bump the major version. It doesn't necessarily mean we need to tag this right away.

@gmlewis
Copy link
Collaborator

gmlewis commented Sep 7, 2019

Oh, OK. Do you have a preference, @willnorris ?

I've been attempting to release&tag after each PR just in case we need to refer to a specific version (so that we don't have to use a commit or PR as our reference), but I can reduce the frequency if you prefer.

I'll go ahead and continue catching up with the outstanding PRs while I've been without internet, and then make a new release and tag, and that should hopefully reduce the noise.

@gmlewis gmlewis merged commit c476c82 into google:master Sep 7, 2019
@gmlewis
Copy link
Collaborator

gmlewis commented Sep 7, 2019

Ah! I see you opened #1280 for discussion. Excellent. We can continue there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Indication that the PR author has signed a Google Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants