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

Merge train #137

Merged
merged 55 commits into from
Aug 17, 2022
Merged

Merge train #137

merged 55 commits into from
Aug 17, 2022

Conversation

rudymatela
Copy link

@rudymatela rudymatela commented Jul 20, 2022

Closes #77.

This is built on top of #134, ..., #155.

  • handle new commit on PR on the train
    • add automated test
  • remove getTrain?
  • handle closing of PRs with pending builds (restart remaining train)
    • add automated test
  • handle overriding remaining train failures with NotIntegrated
    • add automated test
  • handle tagging throughout the train with two tags
    • add automated test
  • handle rebase failures
    • add automated test (base build success)
    • add automated test (base build failure)
  • consider reverting b697ef1 nah!
  • thoroughly test all possible scenarios with 2 PRs (all: one can think of)
  • test some relevant scenarios with 3 PRs.
  • sweep TODOs and refactor ugly code
  • support showing BuildSucceeded in the web interface somehow
  • test in a git-sandbox repository, paste some screenshots here.
  • one more full self code review
  • send this PR for code review
  • merge and tag

Test Case 1, success, success, success

Web before approval

0-web

Web right after approval

1-web

Web final

z-web

PR 120

120

PR 121

121

PR 122

122

@rudymatela rudymatela self-assigned this Jul 20, 2022
@rudymatela rudymatela added the enhancement New feature or request label Jul 20, 2022
@rudymatela
Copy link
Author

rudymatela commented Jul 20, 2022

@OpsBotPrime merge on friday (just a test, should be a no-op)

@rudymatela rudymatela force-pushed the merge-train/isolate-test-branches branch from 273eba5 to e7880bc Compare July 27, 2022 15:22
@rudymatela rudymatela mentioned this pull request Jul 28, 2022
43 tasks
@rudymatela rudymatela force-pushed the merge-train/isolate-test-branches branch from e7880bc to 746472e Compare July 28, 2022 14:09
@rudymatela
Copy link
Author

For reference, this is the old hash before rebasing: ed07d6d

I should probably squash everything at some point to restart.

@OpsBotPrime OpsBotPrime force-pushed the merge-train/isolate-test-branches branch from 746472e to 9cbd57e Compare July 29, 2022 09:57
Base automatically changed from merge-train/isolate-test-branches to master July 29, 2022 09:57
@rudymatela rudymatela force-pushed the merge-train/integration branch 2 times, most recently from 1d0f054 to 23bbe70 Compare July 29, 2022 11:58
@rudymatela
Copy link
Author

For reference, this is the old hash before rebasing: ed07d6d

I should probably squash everything at some point to restart.

Everything is squashed in 23bbe70 now.

@rudymatela rudymatela changed the base branch from master to build-started-status July 29, 2022 16:21
@rudymatela
Copy link
Author

rudymatela commented Jul 29, 2022

Note-to-self:

On Tuesday, carry on from 8e440b0 by running nix run -c stack test --ta '--match "(FOCUS)"' locally to fix the "behind" comment gathering and rendering.

Base automatically changed from build-started-status to master August 2, 2022 08:52
@rudymatela
Copy link
Author

rudymatela commented Aug 10, 2022

There are conflicts again after the latest change.

I will squash everything then rebase on top of the new master.

Since 5139125 is a working version, I temporarily created branch junk/merge-train/integration/1 to serve as a reference.

@rudymatela
Copy link
Author

rudymatela commented Aug 10, 2022

Everything was squashed in 4c7c4ac.

I will rebase this single commit at some point.

@rudymatela
Copy link
Author

Everything was squashed in 4c7c4ac.

I will rebase this single commit at some point.

Rebased as 13df8f0. There weren't as many conflicts due to the squash. :-)

@rudymatela
Copy link
Author

Note-to-self: for adding automated test here:

  • handle overriding remaining train failures with NotIntegrated
    • add automated test

It seems the branch is created on the origin, but not the local somehow does not detect it:

$ git -C /run/user/1000/testsuite-74fda068-fdd1-4bba-97b1-ac6062cd7af2/repo-origin branch -a
  ahead
  alternative
  fixup
  intro
  master
* unused

$ git -C /run/user/1000/testsuite-74fda068-fdd1-4bba-97b1-ac6062cd7af2/repo-origin branch -a
  ahead
  alternative
  fixup
  integration/6
  intro
  master
* unused

$ git -C /run/user/1000/testsuite-74fda068-fdd1-4bba-97b1-ac6062cd7af2/repo-local branch -a
* (HEAD detached from 3b2ca25)
  master
  remotes/origin/master

🤔 💭 This seems to work "for real" but not on the automated tests... Perhaps I should double-check how these repositories are created...

@rudymatela
Copy link
Author

The culprit line is here:

https://github.com/channable/hoff/blob/master/tests/EventLoopSpec.hs#L166-L169

It seems --single-branch does more than cloning just a single branch but rather changes the way git handles the repository so that it avoids keeping track of other branches altogether!

This was hard to find, I threadDelayed before and after the push to check the status of the repositories manually...

@rudymatela
Copy link
Author

rudymatela commented Aug 11, 2022

Before rebase on top of #155: junk/merge-train/integration/2

@rudymatela
Copy link
Author

@ReinierMaas Hey, thanks for the review again. I think I have addressed your suggestions with my last five commits: 4b21410, 44e03dd, a699fd2, 75d9176 and 6b60bd7.

Can you please take a look again?

Copy link

@ReinierMaas ReinierMaas left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for implementing. Now that I have reviewed it I want to see it in action.

Copy link

@RileyApeldoorn RileyApeldoorn left a comment

Choose a reason for hiding this comment

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

Nice work! Really hyped to see this in action :D

I've added some comments and suggestions, some of them we already discussed while going over the code in person (but I'm adding them here for completeness' sake anyway).

src/Logic.hs Outdated Show resolved Hide resolved
src/Logic.hs Outdated Show resolved Hide resolved
src/Logic.hs Show resolved Hide resolved
src/Logic.hs Show resolved Hide resolved
src/Project.hs Show resolved Hide resolved
src/Logic.hs Outdated Show resolved Hide resolved
src/Project.hs Outdated Show resolved Hide resolved
tests/Spec.hs Outdated Show resolved Hide resolved
src/Project.hs Outdated Show resolved Hide resolved
@rudymatela
Copy link
Author

Nice work! Really hyped to see this in action :D

I've added some comments and suggestions, some of them we already discussed while going over the code in person (but I'm adding them here for completeness' sake anyway).

@RileyApeldoorn Thanks for the review. I think I have addressed your comments now.

Copy link

@RileyApeldoorn RileyApeldoorn left a comment

Choose a reason for hiding this comment

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

Nice work!!

Copy link

@alter2000 alter2000 left a comment

Choose a reason for hiding this comment

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

I love this! The code LGTM and matches the logic described on #77 but I'm not sure about the tests as I haven't had enough time to look at them. Thank you Rudy 🥳 !

src/Git.hs Show resolved Hide resolved
src/Logic.hs Show resolved Hide resolved
src/Logic.hs Show resolved Hide resolved
src/Project.hs Outdated Show resolved Hide resolved
@rudymatela
Copy link
Author

@ReinierMaas, @RileyApeldoorn, @alex-mckenna and @alter2000 Thank you all for the detailed reviews. The PR was quite big so I think it was after all useful getting many eyes 👀 to look at this.

I will try to deploy this tomorrow early morning.

@OpsBotPrime merge

@OpsBotPrime
Copy link

Pull request approved for merge by @rudymatela, rebasing now.

Approved-by: rudymatela
Auto-deploy: false
@OpsBotPrime
Copy link

Rebased as 9497f8d, waiting for CI …

@OpsBotPrime
Copy link

CI job started.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request OKR Objectives and Key Results
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Merge train
6 participants