release-toolkit icon indicating copy to clipboard operation
release-toolkit copied to clipboard

Use a Mermaid graph in the backmerge PR body

Open mokagio opened this issue 1 year ago • 4 comments

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:

image
  • Pros: It's neat and colorful and "pro"
  • Cons: It's bound to break or change if/when there's a Mermaid syntax

Checklist before requesting a review

  • [x] Run bundle exec rubocop to test for code style violations and recommendations
  • [x] Add Unit Tests (aka specs/*_spec.rb) if applicable - N.A.
  • [x] Run bundle exec rspec to run the whole test suite and ensure all your tests pass
  • [x] Make sure you added an entry in the CHANGELOG.md file to describe your changes under the appropriate existing ### subsection of the existing ## Trunk section.
  • [x] If applicable, add an entry in the MIGRATION.md file 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.

mokagio avatar Sep 16 '24 21:09 mokagio

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 / as in release/12.3? Or was it -? Can't remember) but that's what made be decide to avoid using Mermaid here in case some custom branch names could sometimes cause trouble.


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 release/xx.y or release/xx.y.z, merge/release-xx.y-into-trunk and trunk/default/main/…; so as long as characters from [a-zA-Z0-9/.-] don't cause trouble (anymore?) that should be ok to use now?

AliSoftware avatar Sep 23 '24 23:09 AliSoftware

I just came here to say: nice!

wzieba avatar Sep 24 '24 08:09 wzieba

as long as characters from [a-zA-Z0-9/.-] don't cause trouble (anymore?) that should be ok to use now?

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)?

iangmaia avatar Oct 02 '24 16:10 iangmaia

as long as characters from [a-zA-Z0-9/.-] don't cause trouble (anymore?) that should be ok to use now?

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? 🤷

AliSoftware avatar Oct 02 '24 16:10 AliSoftware