feat: allow to skip success and fail comments based on provided condition

[JonasSchubert]

Could solve #480 and #636. This is based on the idea of @fgreinacher here.
I added documentation with some highlighting on how to use this functionality and provided some unit tests.

I introduced a new variable defaultComment to ease up the filtering for issues and merge requests, but keeping the default comment message.

semantic-release/github has at least one PR which shall have the same result, but is a different approach. I like the templating idea @fgreinacher brought up and think this would be a good solution there too - also to keep the plugins similar.

I am aware that this project has an open PR. As there was no update since February I took the liberty to create this one, hope this is ok for you @Hakihiro.

[fgreinacher]

I like it a lot, thanks for picking it up @JonasSchubert!

It's a very flexible approach that does not add much complexity to the code base.

@travi @gr2m Any objections on going this way? Would probably be quite easy to handle it the same way in the GitHub plugin (see also semantic-release/github#359 (comment))

[gr2m]

I think the pull request and the feature is great, thank you!

Just one question: it seems that the new defaultComment is meant to do two things

1. decide whether to comment at all based on custom logic
2. customize the comment itself?

In your examples you always use the <%= defaultComment %> placeholder, wrapped in an if condition. Wouldn't it make more sense to introduce an option like commentCondition in which we can just put in the condition string, using the lodash template syntax?

[fgreinacher]

Thanks for the review @gr2m!

It is already possible to disable commenting by statically setting the successComment option to false. Therefore I think it's more consistent to not handle the use case of dynamic disabling differently.

defaultComment is not a config option, but rather a context variable passed to lodash.

[gr2m]

Sorry if I miss something obvious, but why not do an option like condition that can be set to something like "issue.labels?.includes('semantic-release-relevant')" instead?

[fgreinacher]

We could certainly also do that! My only small concern is that we then have two ways two disable the success comment:

1. set the existing successComment option to false.
2. set the new commentCondition option to an expression that evaluates to false.

But it's probably not a big issue if we manage to describe it properly :)

@JonasSchubert WDYT about this idea?

[gr2m]

independent of what we have today, what would the ideal arguments look like? We shouldn't be afraid of breaking things, it's better than ending up with arguments that are unnecessary complicated due to legacy reasons.

Maybe we could deprecate the possibility of setting successComment to false, so that it keeps working but we start logging a deprecation message. And we introduce a new argument like commentCondition which would be the single place to decide whether a comment done or not?

[fgreinacher]

Yes, that sounds like a good plan!

I'm not entirely sure whether lodash templating supports evaluating a boolean expression. That would need some investigation.

[JonasSchubert]

We could certainly also do that! My only small concern is that we then have two ways two disable the success comment:

1. set the existing successComment option to false.
2. set the new commentCondition option to an expression that evaluates to false.

But it's probably not a big issue if we manage to describe it properly :)

@JonasSchubert WDYT about this idea?

Sounds like a good idea to separate the responsibilities of these configuration options. 👍 We had at least three attempts to filter success comments now (semantic-release/github#360, #678 and this PR) and this suggestion sounds reasonable.

Should this then also be changed for the fail step? It has two flags (failComment and failTitle) which can either be false to disable the fail step. Shall this also be handled with a condition or do we keep that logic? I would prefer if we can align the handling of success and fail.

A proper description is definitely needed, but from experience I know description and documentation might not be read by everybody. 😏 This is why I think we have to make it as understandable as possible without the need for too much documentation.

[fgreinacher]

Good point, I agree we should align success and fail comments.

What about successCommentCondition and failCommentCondition?

[gr2m]

Good point, I agree we should align success and fail comments.

What about successCommentCondition and failCommentCondition?

Alternatively we can just make a single commentCondition, but expose context to that conditions to differentiate between success and failer?

[fgreinacher]

Good point, I agree we should align success and fail comments.
What about successCommentCondition and failCommentCondition?

Alternatively we can just make a single commentCondition, but expose context to that conditions to differentiate between success and failer?

I also thought about that, but in the end the reasons for skipping a success comment are quite different from the reasons for skipping a fail comment. The former is usually done to reduce noise, e.g. not spamming irrelevant MRs. The later should IMO only be done in exceptional situations, e.g. when the issue tracker is disabled. I therefore believe mixing both would increase complexity unnecessarily.

[travi]

Would probably be quite easy to handle it the same way in the GitHub plugin (see also semantic-release/github#359 (comment))

sorry for my slow responses here. the last few weeks have been especially crazy for me. i appreciate the callout to this detail. i think this is the biggest consideration for me. like @gr2m, i'm open to breakages to get to an api we are comfortable with and think solves the problem well, but would like to make sure we take both github and gitlab into consideration with that design. i still hope that we can eventually extract some of the common behaviors to a more "git-host core" like package to reduce some of the duplication, but that is something that likely is a bit further into the future still.

[fgreinacher]

Thanks for all your comments!

Let me summarize:

1. we introduce
   a. either a new option commentCondition that can be used to disable both success and fail comments.
   b. or, my favorite, two new options successCommentCondition and failCommentCondition that can be used to disable success and fail comments respectively
2. we log a warning when people are setting successComment. failComment or failTitle to false, announcing this to be removed in future major versions
3. in one of next major releases we can then remove the deprecated functionality

@JonasSchubert Now that the scope has grown a bit are you still willing to work on it?

[JonasSchubert]

Thanks for all your comments!

Let me summarize:

1. we introduce
   a. either a new option commentCondition that can be used to disable both success and fail comments.
   b. or, my favorite, two new options successCommentCondition and failCommentCondition that can be used to disable success and fail comments respectively
2. we log a warning when people are setting successComment. failComment or failTitle to false, announcing this to be removed in future major versions
3. in one of next major releases we can then remove the deprecated functionality

@JonasSchubert Now that the scope has grown a bit are you still willing to work on it?

@fgreinacher I would still be willing to work on it, yes. As soon as we agreed on the way forward I will adjust my PR.

I would then also prefer 1.b. as 1.a. will be very complex to configure for the end user.

[fgreinacher]

@JonasSchubert Great, thanks a ton!

Let's go on with 1b (separate options) then!

[gr2m]

summery sounds good, 1b is good with me, thank you 👍🏼

[fgreinacher]

Thanks a ton for the rework @JonasSchubert and thanks for your patience.

I like it a lot, just some smaller thoughts.

[gr2m]

Just found two typos, other than that this is good to go!

And just making sure, we do not introduce breaking changes, only log a deprecation warning?

Thank you all again for putting so much thought and work into making this feature great

[fgreinacher]

Really great work @JonasSchubert, I love it!

[github-actions]

🎉 This issue has been resolved in version 13.2.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[fgreinacher]

Hey @JonasSchubert, we noticed that you have been super active here for quite some time now and brought in multiple high-quality and impactful contributions.

We'd therefore like to invite you as an additional maintainer for the GitLab plugin.

Would that be interesting for you?

/cc @travi

[JonasSchubert]

Hi @fgreinacher and wow! Thanks for the kind words. I am honored and yes I would be interested.

[travi]

thanks for the help, @JonasSchubert! i've invited you to our gitlab team

[JonasSchubert]

Thanks a lot @travi . Appreciate all your work and I am happy to be psrt of the team!

[fgreinacher]

Welcome @JonasSchubert to the team, awesome to have more people here 🥳