search

semantic-release / gitlab

:fox_face: semantic-release plugin to publish a GitLab release
MIT License
269 stars 78 forks source link

feat(comments): allow to skip success and fail based on provided condition #730

Open JonasSchubert opened 1 month ago

JonasSchubert commented 1 month ago

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 commented 4 weeks ago

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 https://github.com/semantic-release/github/issues/359#issuecomment-2143877794)

fgreinacher commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 (https://github.com/semantic-release/github/pull/360, https://github.com/semantic-release/gitlab/pull/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 commented 2 weeks ago

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

What about successCommentCondition and failCommentCondition?

gr2m commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

@JonasSchubert Great, thanks a ton!

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

gr2m commented 1 week ago

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