Skip to content

Conversation

@randymarsh77
Copy link
Contributor

This PR addresses #17

Draft because: needs testing.

Skip when:

  • The PR is a draft
  • The PR has a 'skip-build' label

Always build when:

  • The PR has a 'build-me' label

@bgrainger
Copy link
Owner

Unfortunately, createNewCommit (which creates the lprb- branches) is called before startBuilds, so this will create a bunch of unnecessary lprb- branches in all the repos (and never* clean them up, because that only happens when a Jenkins build notification is received, which won't occur for Draft PRs).

* Maybe not never; I think if the PR was converted to non-draft, those branches would get moved, then built, then cleaned up? No, that's incorrect. The branches aren't reused; a unique name is created every time.

Skip when:
 - The PR has a 'skip-build' label

Note that this label only controls behavior when the PR is a top-level PR.
@randymarsh77
Copy link
Contributor Author

I've updated the code in accordance with the discussion on the related issue. I've also moved the check earlier in the process to avoid creating commits.

For simplicity:

  • I didn't add a trigger on label change; A PR must be created/opened with the label. If the label is removed, then the rebuild this comment can be used to trigger a build.
  • The logic only checks top-level PRs; It was hard to reason about what return types to use and how to refactor the recursive call into promise chaining... maybe if we convert all the code to async/await it would be easier/safer to make this feature "more robust".

/**
* Checks skip triggers and starts all builds needed for `prId` (which should be in the form 'User/Repo/123') as needed.
*/
function buildPullRequestIfNeeded(prId) {
Copy link
Owner

Choose a reason for hiding this comment

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

buildPullRequest returns a ReplaySubject; buildPullRequestIfNeeded returns a Promise; can those be used interchangeably?

Copy link
Contributor Author

@randymarsh77 randymarsh77 Jul 18, 2022

Choose a reason for hiding this comment

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

It looked like the places I changed to call buildPullRequestIfNeeded weren't using the return value, so I think that shouldn't matter.

*/
function buildPullRequestIfNeeded(prId) {
const [owner, repo, prNumber] = prId.split('/');
return github.repos(owner, repo).pulls(prNumber).fetch()
Copy link
Owner

Choose a reason for hiding this comment

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

Should this Promise be returned? I can't see anything using it. (Or should something "listen" to it?)

} else {
buildPullRequest(prId);
}
});
Copy link
Owner

@bgrainger bgrainger Jul 19, 2022

Choose a reason for hiding this comment

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

Suggested change
});
})
.then(x => null, err => { ... log.error ... });

Should we add a catch-all error handler here to log any errors? Just concerned that failure might be silently dropped on the floor.

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.

2 participants