No-Deploy-Friday-Nudge #56 by xrvdg · Pull Request #95 · channable/hoff

xrvdg · 2021-12-13T17:21:58Z

This pull request adds (non-configurable) No-Deploy-Friday-Nudge functionality (#56) to Hoff.

ReinierMaas

The overall design looks good to me. I just have style pointers but nothing really major. Using the free monad to encapsulate getting the timestamps nicely abstracts it for the testing! And the additional tests also look extensive.

hoff.cabal

src/Logic.hs

ReinierMaas · 2021-12-14T10:40:39Z

src/Logic.hs

+          isApproved <-
+            if isJust approvalType
+              then isReviewer author
+              else pure False
+          day <- dayOfWeek . utctDay <$> getDateTime
+          if isApproved
+            then -- The PR has now been approved by the author of the comment.
+
+            case fromJust approvalType of
+              approval | isMergeOnFriday approval || day /= Friday -> handleCommentAdded' projectConfig prId author state pr approval
+              other -> do
+                () <- leaveComment prId ("Merging is not allowed on Fridays. To override this behaviour use the command `" `Text.append` Pr.displayApproval other `Text.append` " on Friday`.")


Text can be appended with <>. I also see that the lines are quite long we normally restrict this to 100 chars width.

We can combine these multiple if and case statements. When a pull request is approved you know that the fromJust won't fail.

case isApproved of True | your filter here -> handleCommentAdded True -> leave comment False -> pure state

Should we allow merge on Friday on other days?

src/Logic.hs

src/Project.hs

tests/Spec.hs

fatho

Your first PR 🎉 Besides Reinier's remarks, I think the design can also still be simplified/future-proofed (see below).

fatho · 2021-12-14T12:11:40Z

src/Logic.hs

+      -- reversed all "merge and xxx" commands would be detected as a Merge
+      -- command.
+      cases =
+        [ (" merge and deploy on friday", MergeAndDeployOnFriday),


A minor thing: I wonder if "on" is a good choice of words here, as it could be mistaken for a scheduled merge (which we don't support). Maybe "even on"? Not really sure, would otherwise also be fine.

src/Project.hs

ruuda · 2021-12-14T22:25:42Z

src/Logic.hs

+        if isJust approvalType
+          then isReviewer author
+          else pure False
+      day <- dayOfWeek . utctDay <$> getDateTime


This gets the date in the UTC time zone. It will be Friday two hours later in Europe/Amsterdam than in UTC when DST is in effect. Is that intentional? (And also, does it make sense to pick midnight as the boundary? Is Saturday 04:00 an okay time to merge things?)

Thanks for the feedback. What would be your proposal for updating the nudge? Should we detect whether it is Friday Anywhere on Earth? (Start at UTC+14 / End at UTC-12)
Using AoE as the dedicated time range would already disallow the merge to proceed on Thursday afternoon in Europe/Amsterdam when DST is in effect. Which might be a valid moment for the stop merging nudge.

Good point about the timezone! I think just detecting Friday at our location is good enough (maybe the timezone could be made configurable, but that's about it).

Is Saturday 04:00 an okay time to merge things

It is not. But the feature is intended as a guardrail against accidents, not a foolproof method of preventing all untimely merges, and I'd just assume that there won't be accidental merges on Saturday at 04:00.

We might want to just cover the whole weekend, but I think nighttime can be ignored for the moment.

What would be your proposal for updating the nudge?

If it’s just a nudge on Fridays for people working in Europe/Amsterdam, I would just ignore the issue and keep it like this.

At the expense of scope creep, maybe a nicer solution that also covers the weekend is to provide a time range in the configuration file. It could include a start day of week + time of day and an end day of week + time of day, and a time zone in which to interpret them.

fatho

Just some small things from my side:

fatho · 2021-12-16T16:15:00Z

src/Logic.hs

+            pure state
+          (other, OnFriday) -> do
+            () <- leaveComment prId ("Your merge request has been denied because \
+                                      \it not Friday. Run " <> 


s/it not/it is not/

fatho · 2021-12-16T16:15:26Z

src/Logic.hs

+          (other, OnFriday) -> do
+            () <- leaveComment prId ("Your merge request has been denied because \
+                                      \it not Friday. Run " <> 
+                                      Pr.displayApproval other <> " instead")


Good idea rejecting this as well so that people don't get conditioned to just always using the more powerful command 👍

fatho · 2021-12-16T16:26:45Z

src/Logic.hs

+      -- For now Friday at UTC+0 is good enough. 
+      -- See https://github.com/channable/hoff/pull/95 for caveats and improvement ideas.


Can you open a new hoff issue about potentially making this more configurable (e.g. the timezone and the range)?
Then we'll also immediately have a good first issue for the next person joining :-)

ReinierMaas

LGTM! After resolving Fabians comments and one nit. Thanks for implementing this!

ReinierMaas · 2021-12-17T12:37:06Z

src/Logic.hs

+     -- If the pull request is not in the state, ignore the comment.
    Nothing -> pure state

+handleMergeRequested :: ProjectConfiguration -> PullRequestId -> Username -> ProjectState -> PullRequest -> ApprovedFor -> Action ProjectState


Nit: this line is very long.

xrvdg · 2021-12-17T15:13:23Z

@OpsBotPrime merge

OpsBotPrime · 2021-12-17T15:13:24Z

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

Approved-by: xrvdg Auto-deploy: false

OpsBotPrime · 2021-12-17T15:13:29Z

Rebased as 991e117, waiting for CI …

Xander van der Goot added 5 commits December 13, 2021 17:54

Add Free Monad for Time Operations.

b861bf5

Add Merge [and {deploy,tag}] on Friday

8c84490

Add Spec tests for merge [and {tag, deploy}]

ad4a552

Leave comment when merge is denied.

1c85eed

Refactor handleCommentAdded

4753a61

xrvdg assigned ReinierMaas Dec 13, 2021

xrvdg requested a review from ReinierMaas December 13, 2021 17:26

ReinierMaas assigned xrvdg and unassigned ReinierMaas Dec 14, 2021

ReinierMaas requested changes Dec 14, 2021

View reviewed changes

fatho reviewed Dec 14, 2021

View reviewed changes

Xander van der Goot added 4 commits December 14, 2021 16:16

Fix styling issues

eb39d57

Only allow merge on friday on fridays

b5a6551

Break up Approval in Merge types and MergeWindow

d9b8ca9

Use Generator for DateTime in Spec

43ec6cd

ruuda reviewed Dec 14, 2021

View reviewed changes

xrvdg force-pushed the no-deploy-friday branch from 9ff2d56 to bca056f Compare December 16, 2021 10:30

Fixup import order and formatting

246664f

xrvdg force-pushed the no-deploy-friday branch from bca056f to 246664f Compare December 16, 2021 10:31

Comment no deploy friday nudge

0a8d68f

xrvdg force-pushed the no-deploy-friday branch from 06e2040 to 0a8d68f Compare December 16, 2021 10:51

xrvdg requested review from ReinierMaas and fatho December 16, 2021 10:51

channable deleted a comment from ReinierMaas Dec 16, 2021

fatho reviewed Dec 16, 2021

View reviewed changes

ReinierMaas approved these changes Dec 17, 2021

View reviewed changes

Fix no deploy Friday comment.

6e79bcb

xrvdg force-pushed the no-deploy-friday branch from ff60f7f to 6e79bcb Compare December 17, 2021 14:22

Xander van der Goot added 2 commits December 17, 2021 15:51

Fix use fakeRunTime for EventLoopSpec

f95c3a9

Bump hoff version number

c739d45

Merge #95: No-Deploy-Friday-Nudge #56

991e117

Approved-by: xrvdg Auto-deploy: false

OpsBotPrime merged commit 991e117 into master Dec 17, 2021

OpsBotPrime deleted the no-deploy-friday branch December 17, 2021 15:17

xrvdg mentioned this pull request Dec 17, 2021

Make the no-deploy-nudge time window configurable. #96

Open

		-- For now Friday at UTC+0 is good enough.
		-- See https://github.com/channable/hoff/pull/95 for caveats and improvement ideas.

Comments

Conversation

xrvdg commented Dec 13, 2021

Uh oh!

ReinierMaas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fatho left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fatho left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ReinierMaas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xrvdg commented Dec 17, 2021

Uh oh!

OpsBotPrime commented Dec 17, 2021

Uh oh!

OpsBotPrime commented Dec 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants