Skip to content

only trigger conflicts flow for the selected repository#7018

Merged
iAmWillShepherd merged 2 commits intodevelopmentfrom
do-not-trigger-merge-conflicts-in-background
Mar 26, 2019
Merged

only trigger conflicts flow for the selected repository#7018
iAmWillShepherd merged 2 commits intodevelopmentfrom
do-not-trigger-merge-conflicts-in-background

Conversation

@shiftkey
Copy link
Member

@shiftkey shiftkey commented Mar 6, 2019

Overview

Related to #6313

Description

See reproduction steps in #6313 (comment)

I focused on this area because @cheshire137 mentioned that it seemed to occur when dealing with merge conflicts, which was how I stumbled upon the fix below.

I thought this might have been affected by #6141 (first released in 1.6.2-beta1) which introduces the code path:

  • AppStore.withPushPullFetch runs
  • ... which calls AppStore._refreshRepository
  • ... which calls AppStore.loadStatus
  • ... which calls AppStore._triggerConflictsFlow

But the original issue was reported against 1.5.1-beta0 so I'm not sure this is completely resolved.

I also pushed c2e7276 as some cleanup because I couldn't find any callers for that API.

Release notes

Notes: no-notes

@shiftkey shiftkey added the ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Mar 6, 2019
@iAmWillShepherd
Copy link
Contributor

iAmWillShepherd commented Mar 8, 2019

I'm unable to reproduce the behavior this PR fixes, but I'm inclined to improve it anyways because of the cleanup and the added check to make sure we're in the right repo before kicking off background tasks.

@shiftkey
Copy link
Member Author

shiftkey commented Mar 8, 2019

I'm unable to reproduce the behavior this PR fixes,

I'd like someone to sanity check me before this gets merged.

@iAmWillShepherd iAmWillShepherd self-assigned this Mar 21, 2019
@shiftkey shiftkey added this to the 1.7.0 milestone Mar 21, 2019
@iAmWillShepherd iAmWillShepherd dismissed their stale review March 21, 2019 16:23

Want to add reason

iAmWillShepherd
iAmWillShepherd approved these changes Mar 21, 2019
View reviewed changes
Copy link
Contributor

@iAmWillShepherd iAmWillShepherd left a comment

Choose a reason for hiding this comment

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

Reviewed this via zoom so we could walk through reproducing the issue and verifying the fix.

@shiftkey
Copy link
Member Author

shiftkey commented Mar 21, 2019

For extra context, AppStore.shouldBackgroundFetch is rather aggressive in avoiding fetches (it looks at the last_push field for the repository on the GitHub API, so an inactive repository is likely to be skipped) and it's AppStore.performFetch() that then performs AppStore._refreshRepository() which triggers the bug.

@iAmWillShepherd iAmWillShepherd merged commit 5f29c3f into development Mar 26, 2019
@iAmWillShepherd iAmWillShepherd deleted the do-not-trigger-merge-conflicts-in-background branch March 26, 2019 14:08
@niik niik mentioned this pull request Mar 26, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@iAmWillShepherd iAmWillShepherd iAmWillShepherd approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@iAmWillShepherd iAmWillShepherd

Labels

ready-for-review Pull Requests that are ready to be reviewed by the maintainers

Projects

None yet

Milestone

2.0

Development

Successfully merging this pull request may close these issues.

2 participants

@shiftkey @iAmWillShepherd