Use a Mermaid graph in the backmerge PR body #594
Conversation
| commit | ||
| branch #{intermediate_branch} | ||
| checkout #{intermediate_branch} | ||
| commit |
There was a problem hiding this comment.
Technically, at this time, there is no commit on the integration branch. However, without the commit, I don't think the Mermaid graph doesn't convey the idea of the integration branch as well:
without commit
with commit
There was a problem hiding this comment.
Yep, that was also a limitation I remember seeing when I tried it a while ago. It I think it makes sense and is ok to have that commit visually added there anyway.
Technically, at this time, there is no commit on the integration branch
Besides, this is not entirely true. For example, on Tumblr there is a commit done on the intermediate branch after it is cut from release/—the goal of that extra commit being to git checkout $basebranch -- version.xcconfig to auto-resolve the conflict that would otherwise be present on that file due to it evolving separately on release/* branch (for daily betas) vs develop (for daily alphas).
This is also why this Fastlane action has a ConfigItem allowing you to pass a Proc to add arbitrary commits on the intermediate branch after it's cut but before the PR is created, btw 😉
There was a problem hiding this comment.
IMO the fonts and the overall size of the chart look too big for me (or maybe I'm too used to the ASCII art 😄 ), taking a lot of space of the PR text block. Is there a way to make it a bit smaller?
There was a problem hiding this comment.
It also feels strange that the commits on trunk seem to have happened only after the commits from release/4.53 in the screenshot above; It's like if the 3 commits from trunk that were illustrated in this Mermaid graph were only pushed after release/* and merge/release-*-into-* had been created, instead of suggesting that the trunk branch has been existing and living its life for way longer before release/* branch… 🤔
| <details> | ||
| <summary>Expand to see an ASCII representation of the Git graph above</summary> | ||
|
|
||
| #{ascii_git_graph} | ||
|
|
||
| </details> |
There was a problem hiding this comment.
Maybe we can mitigate the "disruption" of the graph not rendering correctly in GitHub for whichever reason by having the ASCII as a fallback here.
|
To be honest I remember trying using Mermaid for that a while back (I think at the time I initially introduced the ASCII art in backmerge PRs a while ago actually, as I tried to do that using Mermaid first) The issues I remember having back then is that some characters in some branch names broke the mermaid interpreter—which was then unable to render the graph at all. I don't remember what were those problematic characters that caused this during my testing back then (maybe That being said, maybe they fixed that bug since (it was quite some time ago, after all); or maybe such issue won't occur in the context of this backmerge-pr action because the branch names are already deterministic (should all be in the form of |
...lane/plugin/wpmreleasetoolkit/actions/common/create_release_backmerge_pull_request_action.rb
Outdated
Show resolved
Hide resolved
|
I just came here to say: nice! |
| GRAPH | ||
|
|
||
| pr_body = <<~BODY | ||
| Merging `#{head_branch}` into `#{base_branch}`. |
There was a problem hiding this comment.
wdyt about adding the chart type as a parameter? Something like :image, :ascii, :both.
There was a problem hiding this comment.
My first instinct was to say thatI'd rather always have both in case the Mermaid one fails, which is something we cannot know at the time we generated the body because it depends on GitHub.
But, it's good to build flexible systems (unopinionated tools, opinionated golden path) and the adding a parameter like this does only makes the code a tiny bit more complex (simple is usually better).
I'll update 👍
There was a problem hiding this comment.
I'll update 👍
Spawned #596 to not slow this down while waiting for that.
This seems to be the case as the images @mokagio generated so far contained these characters (I think the capital letters are the only exception)? |
Yeah, seems they fixed the bug since last time I tried it? 🤷 |
What does it do?
I was waiting for CI to finish on a backmerge PR looking at the ASCII art describing the merge and got side tracked into this nerd indulgence:
Checklist before requesting a review
bundle exec rubocopto test for code style violations and recommendationsspecs/*_spec.rb) if applicable - N.A.bundle exec rspecto run the whole test suite and ensure all your tests passCHANGELOG.mdfile to describe your changes under the appropriate existing###subsection of the existing## Trunksection.MIGRATION.mdfile to describe how the changes will affect the migration from the previous major version and what the clients will need to change and consider. - N.A.