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

remove blob_href from check annotations #1242

Merged
merged 3 commits into from
Sep 7, 2019

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.

None yet

5 participants