Report URLs of pending status checks #138

alex-mckenna · 2022-07-21T14:15:56Z

This PR allows hoff to report the target_url of pending builds, giving users an indication of which job hoff is waiting for. This is shown both as a comment on the PR, and an additional link in the headline for pull requests where a CI URL is available (i.e. pending and failed jobs, since we don't currently keep the URL of succeeded jobs).

Screenshots:

I'm slightly unsure if this will actually go off in practice. This info is given when the build status changes to pending with a non-null target_url, but I have no concrete intuition for when this would occur in practice. I'm hoping when the status check starts it will change the state from BuildPending Nothing to BuildPending (Just url).

Closes #26
Closes #109

rudymatela

Nice. Other than the typo and minor indenting issues I found, looks good to me. 👍

rudymatela · 2022-07-21T14:34:55Z

src/Logic.hs

+            -- Sometimes we want to leave an informative comment on the PR. This isn't for things
+            -- that require uesr feedback like build failures.


itym user:

Suggested change

-- Sometimes we want to leave an informative comment on the PR. This isn't for things

-- that require uesr feedback like build failures.

-- Sometimes we want to leave an informative comment on the PR. This isn't for things

-- that require user feedback like build failures.

rudymatela · 2022-07-21T14:38:42Z

src/WebInterface.hs

+    case integrationStatus pullRequest of
+      Integrated _ (BuildPending (Just ciUrl)) -> do
+        span "  | "
+        a ! href (toValue ciUrl) $ "View in CI"
+
+      Integrated _ (BuildFailed (Just ciUrl)) -> do
+        span "  | "
+        a ! href (toValue ciUrl) $ "View in CI"
+
+      _ -> pure ()


Awesome... and pretty handy!

~~While reading the original issues I found something to improve.~~

~~Instead of just "View in CI", maybe it would be better to have: "🟡 build pending" and "❌ build failed " as the link labels for each case. So one can see the status without clicking the links.~~

NVM, the PRs are already separated by category.

rudymatela · 2022-07-21T14:41:20Z

tools/send-webhook

 	event=
 	commit="$1"
 	state="$2"
+        target="$3"


Code is misaligned here due to a mix of tabs and spaces. Changing the 8 spaces before target to a tab fixes it:

Suggested change

event=

commit="$1"

state="$2"

target="$3"

event=

commit="$1"

state="$2"

target="$3"

rudymatela · 2022-07-21T14:43:01Z

tools/send-webhook

 	[ -n "$state" ] || state=success

+        # If this is a URL make sure it has quote marks, if null make sure it doesn't.
+        if [ -z "$target" ]; then
+                target="null"
+        else
+                target="\"$target\""
+        fi
+
 	event="status"


Code is misaligned here as well due to spaces instead of tabs:

Suggested change

[ -n "$state" ] || state=success

# If this is a URL make sure it has quote marks, if null make sure it doesn't.

if [ -z "$target" ]; then

target="null"

else

target="\"$target\""

fi

event="status"

[ -n "$state" ] || state=success

# If this is a URL make sure it has quote marks, if null make sure it doesn't.

if [ -z "$target" ]; then

target="null"

else

target="\"$target\""

fi

event="status"

rudymatela · 2022-07-21T14:47:57Z

tests/Spec.hs

-      event `shouldBe` (BuildStatusChanged (Sha "b26354") Project.BuildPending)
+      event `shouldBe` (BuildStatusChanged (Sha "b26354") (Project.BuildPending (Just "https://travis-ci.org/rachael/owl/builds/1982")))


Nice. The test fixture was already there. :-)

rudymatela · 2022-07-21T14:54:17Z

src/WebInterface.hs

+    case integrationStatus pullRequest of
+      Integrated _ (BuildPending (Just ciUrl)) -> do
+        span "  | "
+        a ! href (toValue ciUrl) $ "View in CI"
+
+      Integrated _ (BuildFailed (Just ciUrl)) -> do
+        span "  | "
+        a ! href (toValue ciUrl) $ "View in CI"
+
+      _ -> pure ()


~~While reading the original issues I found something to improve.~~

~~Instead of just "View in CI", maybe it would be better to have: "🟡 build pending" and "❌ build failed " as the link labels for each case. So one can see the status without clicking the links.~~

NVM, the PRs are already separated by category.

rudymatela · 2022-07-21T14:59:06Z

I'm slightly unsure if this will actually go off in practice. This info is given when the build status changes to pending with a non-null target_url, but I have no concrete intuition for when this would occur in practice. I'm hoping when the status check starts it will change the state from BuildPending Nothing to BuildPending (Just url).

For this, I think GitHub sends a BuildPending hook again with the build URL when it gets one. But I think you've already figured this out because I saw your changes on send-webhook which deal with this.

rudymatela

LGTM! (modulo minor fixes)

rudymatela · 2022-07-21T15:11:32Z

Btw, I reverted the broken merge of yesterday. So feel free to rebase and merge this into master now if you like.

I saw this created a few conflicts with your files (as your change was already based on top of mine). I'm sorry about that 😔. I reviewed the conflicts though, and they seem to be not major. :-)

alex-mckenna · 2022-07-21T15:38:33Z

Okay, the rebase was mostly mechanical apart from the handleBuildStatusChanged which was a little different after the revert for some reason. I changed the PR to be in the style of the reverted version so give it a once over in case anything dumb crept in

rudymatela

LGTM.

rudymatela · 2022-07-22T07:18:45Z

src/Logic.hs

+        -- Sometimes we want to leave an informative comment on the PR. This isn't for things
+        -- that require uesr feedback like build failures.


s/uesr/user/

Suggested change

-- Sometimes we want to leave an informative comment on the PR. This isn't for things

-- that require uesr feedback like build failures.

-- Sometimes we want to leave an informative comment on the PR. This isn't for things

-- that require user feedback like build failures.

in case anything dumb crept in

Hey look, something dumb

alex-mckenna · 2022-07-22T13:32:23Z

@OpsBotPrime merge on friday

OpsBotPrime · 2022-07-22T13:32:25Z

Pull request approved for merge by @alex-mckenna, rebasing now.

OpsBotPrime · 2022-07-22T13:32:29Z

Rebased as b4994ca, waiting for CI …

alex-mckenna requested a review from rudymatela July 21, 2022 14:15

rudymatela reviewed Jul 21, 2022

View reviewed changes

rudymatela requested review from rudymatela and removed request for rudymatela July 21, 2022 14:51

rudymatela suggested changes Jul 21, 2022

View reviewed changes

rudymatela self-requested a review July 21, 2022 15:01

rudymatela approved these changes Jul 21, 2022

View reviewed changes

alex-mckenna force-pushed the report-ci-run-link branch from dfa71be to 517632b Compare July 21, 2022 15:36

alex-mckenna requested a review from rudymatela July 21, 2022 15:38

rudymatela approved these changes Jul 22, 2022

View reviewed changes

rudymatela assigned alex-mckenna Jul 22, 2022

rudymatela added the enhancement New feature or request label Jul 22, 2022

Report URLs of pending status checks

b4994ca

OpsBotPrime force-pushed the report-ci-run-link branch from 06c6812 to b4994ca Compare July 22, 2022 13:38

OpsBotPrime merged commit b4994ca into master Jul 22, 2022

OpsBotPrime deleted the report-ci-run-link branch July 22, 2022 13:38

rudymatela mentioned this pull request Jul 22, 2022

Test the build status comment. #143

Merged

rudymatela mentioned this pull request Aug 2, 2022

Duplicate "CI job" messages #147

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Report URLs of pending status checks #138

Report URLs of pending status checks #138

alex-mckenna commented Jul 21, 2022

rudymatela left a comment

rudymatela Jul 21, 2022

rudymatela Jul 21, 2022

rudymatela Jul 21, 2022 •

edited

rudymatela Jul 21, 2022

rudymatela Jul 21, 2022

rudymatela Jul 21, 2022

rudymatela Jul 21, 2022 •

edited

rudymatela commented Jul 21, 2022

rudymatela left a comment

rudymatela commented Jul 21, 2022

alex-mckenna commented Jul 21, 2022

rudymatela left a comment

rudymatela Jul 22, 2022

alex-mckenna Jul 22, 2022

rudymatela Jul 22, 2022

alex-mckenna commented Jul 22, 2022

OpsBotPrime commented Jul 22, 2022

OpsBotPrime commented Jul 22, 2022

		-- Sometimes we want to leave an informative comment on the PR. This isn't for things
		-- that require uesr feedback like build failures.

		event `shouldBe` (BuildStatusChanged (Sha "b26354") Project.BuildPending)
		event `shouldBe` (BuildStatusChanged (Sha "b26354") (Project.BuildPending (Just "https://travis-ci.org/rachael/owl/builds/1982")))

Report URLs of pending status checks #138

Report URLs of pending status checks #138

Conversation

alex-mckenna commented Jul 21, 2022

rudymatela left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rudymatela Jul 21, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rudymatela Jul 21, 2022 • edited

Choose a reason for hiding this comment

rudymatela commented Jul 21, 2022

rudymatela left a comment

Choose a reason for hiding this comment

rudymatela commented Jul 21, 2022

alex-mckenna commented Jul 21, 2022

rudymatela left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-mckenna commented Jul 22, 2022

OpsBotPrime commented Jul 22, 2022

OpsBotPrime commented Jul 22, 2022

rudymatela Jul 21, 2022 •

edited

rudymatela Jul 21, 2022 •

edited