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

Split BuildPending into BuildStarted. #146

Merged
merged 8 commits into from
Aug 2, 2022
Merged

Conversation

rudymatela
Copy link

@rudymatela rudymatela commented Jul 29, 2022

In preparation for: #137.
Closes: #77.

This changes how the status is represented to something more explicit and descriptive:

BuildPending Nothing                     -> BuildPending
BuildPending (Just "ci.example.com/...") -> BuildStarted "ci.example.com/..."

This will make the change in #137 much nicer and simpler.

I checked and this does not affect the parsing of status files during an upgrade.

There are two other somewhat unrelated changes explained below.

Build status wording

... and slightly change the build status comment wording to "CI job started."

Rendering Status + Commit Hash on the web interface

... and change web rendering of CI jobs. Besides the link to the CI, we also now include a link to the commit in GitHub.

Before:

old

After:

new

The icons are clickable and so are the commit hashes:

Even though the heading already identifies the build status, this makes it so that the link to CI is somewhat consistent with GitHub UX:
gh-interface

Having "View in CI | e711b80" looked a bit weird now that the commit hash is present. Not applicable anymore.

Here is the change:

BuildPending Nothing              -> BuildPending
BuildStarted (Just "example.com") -> BuildStarted "example.com"

... and slightly change the comment wording to "CI job started."
@rudymatela rudymatela self-assigned this Jul 29, 2022
@rudymatela rudymatela mentioned this pull request Jul 29, 2022
43 tasks
@rudymatela rudymatela marked this pull request as ready for review July 29, 2022 16:13
Copy link

@alex-mckenna alex-mckenna left a comment

Choose a reason for hiding this comment

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

One nit, otherwise looks good

Comment on lines +240 to +243
(BuildStarted ciUrl) -> ciLink ciUrl "🟡"
(BuildFailed (Just ciUrl)) -> ciLink ciUrl "❌"
_ -> pure ()
a ! href (toValue $ commitUrl info sha) $ toHtml $ prettySha sha

Choose a reason for hiding this comment

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

I think I would change this, having the link to CI only be shown as an icon makes it easier to miss. If I didn't realise I would probably first click the SHA and possibly miss the fact the icon is a completely different link.

Copy link
Author

Choose a reason for hiding this comment

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

Fair enough. I'll include a "CI build" link besides the hash when it is available.

@rudymatela
Copy link
Author

UI as of 82ecce5:

intermediary

@rudymatela
Copy link
Author

UI as of febcaa2:
new

The PR description has been updated with this.

... and here's how it looks before the build link arrives:

building-no-ci

@rudymatela
Copy link
Author

@OpsBotPrime merge yourself please and thank you.

@OpsBotPrime
Copy link

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

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

Rebased as 12b01d1, waiting for CI …

@OpsBotPrime OpsBotPrime merged commit 12b01d1 into master Aug 2, 2022
@OpsBotPrime OpsBotPrime deleted the build-started-status branch August 2, 2022 08:52
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.

Merge train
3 participants