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

feat: warn if autoupdate fails on a fork #219

Merged
merged 4 commits into from
Sep 12, 2021
Merged

Conversation

chinthakagodawita
Copy link
Owner

@chinthakagodawita chinthakagodawita commented Aug 28, 2021

Instead of erroring out the Actions run, this change will make autoupdate gracefully handle updates against forks that it doesn't have write access to.

Also in this change:

  • Update the Ubuntu version in the README to match the latest LTS

Example here: https://github.com/chinthakagodawita/autoupdate-test/actions/runs/1176318083 (notice the error annotation instead of a failed build)

Fixes #212

@codecov
Copy link

codecov bot commented Aug 28, 2021

Codecov Report

Merging #219 (4ea966d) into master (3cc261e) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #219   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            3         3           
  Lines          565       593   +28     
  Branches        84        88    +4     
=========================================
+ Hits           565       593   +28     
Impacted Files Coverage Δ
src/autoupdater.ts 100.00% <100.00%> (ø)

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 3cc261e...4ea966d. Read the comment docs.

@chinthakagodawita chinthakagodawita marked this pull request as ready for review August 28, 2021 03:28
(e as RequestError).status === 403 &&
sourceEventOwner !== mergeOpts.owner
) {
ghCore.error(
Copy link
Collaborator

Choose a reason for hiding this comment

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

do you think it would be useful to include github's actual error message here as well?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Might be too verbose for the annotation? Though, I'll add it in as a debug message, might be helpful for figuring out issues

Copy link
Owner Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah I think that's good

@anarast
Copy link
Collaborator

anarast commented Aug 30, 2021

unrelated to this PR, but I noticed in your autoupdate-test run that the successful PR updates didn't log out the success message Branch update successful, new branch HEAD ... - is that expected?

The Github API now returns a HTTP 201 on successful merge.
@chinthakagodawita
Copy link
Owner Author

chinthakagodawita commented Aug 31, 2021

unrelated to this PR, but I noticed in your autoupdate-test run that the successful PR updates didn't log out the success message Branch update successful, new branch HEAD ... - is that expected?

Good catch! Looks like the API returns a HTTP 201 now: https://docs.github.com/en/rest/reference/repos#merging

Updated in 4ea966d

Example run here: https://github.com/chinthakagodawita/autoupdate-test/runs/3471509330?check_suite_focus=true

Copy link
Collaborator

@anarast anarast left a comment

Choose a reason for hiding this comment

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

👍

@chinthakagodawita chinthakagodawita merged commit 0e607a8 into master Sep 12, 2021
@chinthakagodawita chinthakagodawita deleted the fork-messaging branch September 27, 2021 09:57
@chinthakagodawita
Copy link
Owner Author

Released in v1.6.0

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.

Caught error trying to update branch: Resource not accessible by integration
2 participants