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.

Sign up for GitHub

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

Use a Mermaid graph in the backmerge PR body #594

Open
wants to merge 4 commits into
base: trunk
Choose a base branch
from
Open

Use a Mermaid graph in the backmerge PR body #594

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
205fc54
Use a Mermaid graph in the backmerge PR body
mokagio Sep 16, 2024
a9a887e
Use a `<details>` node to reintroduce ASCII git graph in backmerge
mokagio Sep 16, 2024
4c14ca6
Add changelog entry for Mermaid git graph in backmerge PR
mokagio Sep 16, 2024
d5ed207
Fix Mermaid config using hardcoded `trunk`
mokagio Sep 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 44 additions & 3 deletions ...e/plugin/wpmreleasetoolkit/actions/common/create_release_backmerge_pull_request_action.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,17 +110,58 @@ def self.create_backmerge_pr(token:, repository:, title:, head_branch:, base_bra

other_action.push_to_git_remote(tags: false)

pr_body = <<~BODY
Merging `#{head_branch}` into `#{base_branch}`.
# Live playground to edit this graph
# https://mermaid.live/edit#pako:eNqNkU1PwzAMhv9KZanqpWVFwCVHQOKCxoFrLl7itdGaZEoTTWjqf8eLOo2i8eGTPx77TewjKK8JBJTlUbqCzTgTRTEHJ6s6E18C7vtqkc4li8Y9BnSqX6MlBqoYkttV9Y_cW9AUGLxt2_Y7Nfb-8OStNfEVNzQwtcVhpCU1XUJ2p7KU7vzAXNlkmSLQQDjS6v7m4S7nVU9q51O8UsmSv7jzSEuho9Xc3pzaG-Oib_KXlxr_QL8onbuuVy9unvrXbKiBCV645qvme0mIPVmSINjVtMU0RAm8O0YxRf_-4RQIbqca0l5jpGeDXUALIu9_-gRejKzU
meramid_git_graph = <<~GRAPH
```mermaid
%%{
init: {
'gitGraph': {
'mainBranchName': 'trunk',
'mainBranchOrder': 1000,
'showCommitLabel': false
}
}
}%%
gitGraph
branch #{head_branch}
checkout #{head_branch}
commit
commit
commit
branch #{intermediate_branch}
checkout #{intermediate_branch}
commit
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

image

with commit

image

Copy link
Contributor

Choose a reason for hiding this comment

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

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 😉

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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… 🤔

checkout #{base_branch}
commit
commit
merge #{base_branch}
```
GRAPH

Via intermediate branch `#{intermediate_branch}`, to help fix conflicts if any:
ascii_git_graph = <<~GRAPH
```
#{head_branch.rjust(40)} ----o-- - - -
#{' ' * 40} \\
#{intermediate_branch.rjust(40)} `---.
#{' ' * 40} \\
#{base_branch.rjust(40)} ------------x- - -
```
GRAPH

pr_body = <<~BODY
Merging `#{head_branch}` into `#{base_branch}`.
Copy link
Contributor

Choose a reason for hiding this comment

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

wdyt about adding the chart type as a parameter? Something like :image, :ascii, :both.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll update 👍

Spawned #596 to not slow this down while waiting for that.


Via intermediate branch `#{intermediate_branch}`, to help fix conflicts if any:

#{meramid_git_graph}

<details>
<summary>Expand to see an ASCII representation of the Git graph above</summary>

#{ascii_git_graph}

</details>
Comment on lines +159 to +164
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

BODY

other_action.create_pull_request(
Expand Down