Minimize latest plan preview comment before comment error message #4683

TakumaKurosawa · 2023-11-22T08:00:27Z

What this PR does / why we need it:

The status badge appear twice

Minimize previous comment when plan-preview result has error

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

How are users affected by this change:
Is this breaking change:
How to migrate (if breaking change):

ffjlabo · 2023-11-22T10:10:42Z

@TakumaKurosawa
Thank you for the PR!
I would like to recheck the purpose!

You want to do below, right?

fix not to show the status badge twice
minimize the previous comment not only when the plan-preview succeed but also the plan-preview result has an error (because not minimized before)

TakumaKurosawa · 2023-11-22T10:50:23Z

@ffjlabo
Thanks for checking my pull request.
Your thoughts are completely in line with mine.

khanhtc1202 · 2023-11-22T11:01:29Z

tool/actions-plan-preview/main.go

+		if comment == nil {
+			log.Println("plan-preview result has error")
+			os.Exit(1)
+			return
+		}


What if there is no previous comment 🤔

@khanhtc1202
I'm sorry for replying later.
Nothing to minimize any message if there is no previous comment.
I'm not sure what you want to know, sorry.

I mean in that case, this comment would be nil and I guess we can return normally, no need to exit with error here, isn't it? 🤔

@khanhtc1202
This comment is latest previous comment that must close.
But if result.HasError() {} is still true, so we have to exit with error.

@khanhtc1202 cc @TakumaKurosawa
It should stop as exit 1 because the fix is in the error case in which plan-preview failed.

So if the previous comment doesn't exist, the code wants to finish as exit 1 with any errors when the plan-preview result has some errors.
This is like an early return.

This is confusing, so he created a function for minimizing a previous comment 👍
This function does not return errors if some errors occur in it because we should comment on the PR about the plan-preview result regardless of failing to minimize
74a9d8e

Got your points, thank both 🙏

ffjlabo · 2023-11-28T02:11:08Z

tool/actions-plan-preview/main.go

+		// Find comments we sent before
+		comment, err := findLatestPlanPreviewComment(ctx, ghGraphQLClient, event.Owner, event.Repo, event.PRNumber)
+		if err != nil {
+			log.Printf("Unable to find the previous comment to minimize (%v)\n", err)
+		}
+
 		body := makeCommentBody(event, result)
-		doComment(failureBadgeURL + "\n" + body)
+		doComment(body)
+
+		if comment == nil {
+			log.Println("plan-preview result has error")
+			os.Exit(1)
+			return
+		}
+
+		if bool(comment.IsMinimized) {
+			log.Printf("Previous plan-preview comment has already minimized. So don't minimize anything\n")
+			return
+		}
+
+		if err := minimizeComment(ctx, ghGraphQLClient, comment.ID, "OUTDATED"); err != nil {
+			log.Printf("warning: cannot minimize comment: %s\n", err.Error())
+			return
+		}


[IMO]

minimize the previous comment not only when the plan-preview succeed but also the plan-preview result has an error (because not minimized before)

To realize this, It would be nice to gather these process into one method 👀

find latest comment

makeCommentBody

do comment

minimizeComment

Then we only call the method regardless of failure or success. WDYT?

@ffjlabo
Yes! Exactly!
I will gather these process.

@ffjlabo
Applied!!
74a9d8e

@TakumaKurosawa Sorry for late reply 🙏 It looks good 👍

codecov · 2023-12-03T10:03:35Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f7287c8) 30.82% compared to head (57b3af0) 30.83%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #4683   +/-   ##
=======================================
  Coverage   30.82%   30.83%           
=======================================
  Files         221      221           
  Lines       25993    25993           
=======================================
+ Hits         8012     8014    +2     
+ Misses      17331    17329    -2     
  Partials      650      650

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

khanhtc1202 · 2023-12-07T04:45:41Z

@ffjlabo please re-check 👀

ffjlabo

@TakumaKurosawa
Thank you! just one more nit point 🙏
If this fixed I will approve

ffjlabo · 2023-12-07T04:56:23Z

tool/actions-plan-preview/main.go

-		doComment(failureBadgeURL + "\n" + body)
+		doComment(body)
+
+		log.Printf("Successfully minimized last plan-preview result on pull request\n")


log.Printf -> log.Println would be better 👀

@ffjlabo
Applied!
f286ae2

ffjlabo · 2023-12-07T04:56:48Z

tool/actions-plan-preview/planpreview.go

+		log.Printf("Previous plan-preview comment has already minimized. So don't minimize anything\n")
+		return


same here 🙏

ffjlabo · 2023-12-07T04:57:13Z

tool/actions-plan-preview/planpreview.go

+		return
+	}
+
+	log.Printf("Successfully minimized last plan-preview result on pull request\n")


same here 🙏

ffjlabo

Thank you for your contribution! 🚀

Signed-off-by: Takumaron <[email protected]>

Signed-off-by: TakumaKurosawa <[email protected]>

khanhtc1202

Here you go 🪨

khanhtc1202 · 2023-12-07T11:12:17Z

@TakumaKurosawa The new version of action-plan-preview had been released, thank you 🙏
https://github.com/pipe-cd/actions-plan-preview/releases/tag/v1.7.6

…pe-cd#4683) * Minimize latest plan preview comment before comment error message Signed-off-by: Takumaron <[email protected]> * Create minimize previous comment function Signed-off-by: TakumaKurosawa <[email protected]> * Use log.Println instead of log.Printf Signed-off-by: TakumaKurosawa <[email protected]> --------- Signed-off-by: Takumaron <[email protected]> Signed-off-by: TakumaKurosawa <[email protected]>

…pe-cd#4683) * Minimize latest plan preview comment before comment error message Signed-off-by: Takumaron <[email protected]> * Create minimize previous comment function Signed-off-by: TakumaKurosawa <[email protected]> * Use log.Println instead of log.Printf Signed-off-by: TakumaKurosawa <[email protected]> --------- Signed-off-by: Takumaron <[email protected]> Signed-off-by: TakumaKurosawa <[email protected]> Signed-off-by: sZma5a <[email protected]>

…pe-cd#4683) * Minimize latest plan preview comment before comment error message Signed-off-by: Takumaron <[email protected]> * Create minimize previous comment function Signed-off-by: TakumaKurosawa <[email protected]> * Use log.Println instead of log.Printf Signed-off-by: TakumaKurosawa <[email protected]> --------- Signed-off-by: Takumaron <[email protected]> Signed-off-by: TakumaKurosawa <[email protected]> Signed-off-by: 鈴木優耀 <[email protected]>

TakumaKurosawa requested review from nghialv, khanhtc1202, kentakozuka and ffjlabo as code owners November 22, 2023 08:00

github-actions bot added the area/tool label Nov 22, 2023

khanhtc1202 reviewed Nov 22, 2023

View reviewed changes

TakumaKurosawa force-pushed the fix-plan-preview-error-comment branch from 2878653 to 0bff295 Compare November 28, 2023 01:45

TakumaKurosawa requested a review from khanhtc1202 November 28, 2023 01:51

ffjlabo reviewed Nov 28, 2023

View reviewed changes

TakumaKurosawa requested a review from ffjlabo December 1, 2023 04:26

TakumaKurosawa force-pushed the fix-plan-preview-error-comment branch from 74a9d8e to 84dcc11 Compare December 6, 2023 01:27

ffjlabo reviewed Dec 7, 2023

View reviewed changes

TakumaKurosawa requested a review from t-kikuc as a code owner December 7, 2023 07:06

TakumaKurosawa force-pushed the fix-plan-preview-error-comment branch from 1cb42fa to f286ae2 Compare December 7, 2023 07:07

ffjlabo approved these changes Dec 7, 2023

View reviewed changes

TakumaKurosawa added 3 commits December 7, 2023 16:32

Minimize latest plan preview comment before comment error message

2c858d0

Signed-off-by: Takumaron <[email protected]>

Create minimize previous comment function

96162cf

Signed-off-by: TakumaKurosawa <[email protected]>

Use log.Println instead of log.Printf

57b3af0

Signed-off-by: TakumaKurosawa <[email protected]>

ffjlabo force-pushed the fix-plan-preview-error-comment branch from f286ae2 to 57b3af0 Compare December 7, 2023 07:32

khanhtc1202 approved these changes Dec 7, 2023

View reviewed changes

khanhtc1202 merged commit 2501848 into pipe-cd:master Dec 7, 2023
15 checks passed

TakumaKurosawa deleted the fix-plan-preview-error-comment branch December 7, 2023 08:15

khanhtc1202 mentioned this pull request Dec 7, 2023

Update action-plan-preview image pipe-cd/actions-plan-preview#17

Merged

github-actions bot mentioned this pull request Feb 6, 2024

Release v0.46.0 #4775

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minimize latest plan preview comment before comment error message #4683

Minimize latest plan preview comment before comment error message #4683

TakumaKurosawa commented Nov 22, 2023 •

edited

Loading

ffjlabo commented Nov 22, 2023 •

edited

Loading

TakumaKurosawa commented Nov 22, 2023

khanhtc1202 Nov 22, 2023

TakumaKurosawa Nov 28, 2023

khanhtc1202 Nov 28, 2023

TakumaKurosawa Dec 1, 2023

ffjlabo Dec 6, 2023

ffjlabo Dec 6, 2023 •

edited

Loading

khanhtc1202 Dec 7, 2023

ffjlabo Nov 28, 2023

TakumaKurosawa Dec 1, 2023

TakumaKurosawa Dec 1, 2023

ffjlabo Dec 5, 2023

codecov bot commented Dec 3, 2023 •

edited

Loading

khanhtc1202 commented Dec 7, 2023

ffjlabo left a comment

ffjlabo Dec 7, 2023

TakumaKurosawa Dec 7, 2023 •

edited

Loading

ffjlabo Dec 7, 2023

TakumaKurosawa Dec 7, 2023 •

edited

Loading

ffjlabo Dec 7, 2023

TakumaKurosawa Dec 7, 2023 •

edited

Loading

ffjlabo left a comment

khanhtc1202 left a comment

khanhtc1202 commented Dec 7, 2023

		log.Printf("Previous plan-preview comment has already minimized. So don't minimize anything\n")
		return

Minimize latest plan preview comment before comment error message #4683

Minimize latest plan preview comment before comment error message #4683

Conversation

TakumaKurosawa commented Nov 22, 2023 • edited Loading

ffjlabo commented Nov 22, 2023 • edited Loading

TakumaKurosawa commented Nov 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ffjlabo Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 3, 2023 • edited Loading

Codecov Report

khanhtc1202 commented Dec 7, 2023

ffjlabo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TakumaKurosawa Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TakumaKurosawa Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TakumaKurosawa Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

ffjlabo left a comment

Choose a reason for hiding this comment

khanhtc1202 left a comment

Choose a reason for hiding this comment

khanhtc1202 commented Dec 7, 2023

TakumaKurosawa commented Nov 22, 2023 •

edited

Loading

ffjlabo commented Nov 22, 2023 •

edited

Loading

ffjlabo Dec 6, 2023 •

edited

Loading

codecov bot commented Dec 3, 2023 •

edited

Loading

TakumaKurosawa Dec 7, 2023 •

edited

Loading

TakumaKurosawa Dec 7, 2023 •

edited

Loading

TakumaKurosawa Dec 7, 2023 •

edited

Loading