feat: add table output to `diff` #2454

hugorut · 2023-05-08T12:27:09Z

Adds summary table to diff output to improve understanding about cost diffs between projects. This table only shows projects with cost diffs. e.g:

┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━┳━━━━━━━━━━━━┳━━━━━━━━━━━━━━┓
┃ Name                                                                 ┃ Previous   ┃ New        ┃ Diff         ┃
┣━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╋━━━━━━━━━━━━╋━━━━━━━━━━━━╋━━━━━━━━━━━━━━┫
┃ infracost/infracost/cmd/infracost/testdata                           ┃ $0.00      ┃ $1,361     ┃ +$1,361      ┃
┃ infracost/infracost/cmd/infracost/testdata/terraform_v0.14_plan.json ┃ $41        ┃ $81        ┃ +$41 (+100%) ┃
┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┻━━━━━━━━━━━━┻━━━━━━━━━━━━┻━━━━━━━━━━━━━━┛
2 projects had no cost estimate changes, rerun with --show-skipped to see details

Zero diff projects are ignored to make this consistent with GitHub comments. This PR also:

removes the verbose "no cost estimate change" line to just be a count unless --show-skipped is included
reformats the errored projects to be above the table so that the table is explicitly the last output

Adds summary table to diff output to improve understanding about cost diffs between projects. This table only shows porjects with cost diffs. Zero diff projects are ignored to make this consistent with GitHub comments. Along with the table this PR also: * removes the verbose "no cost estimate change" line to just be a count unless --show-skipped is included * reformats the errored projects to be above the table so that the table is explicitly the last output

aliscott · 2023-05-08T14:14:08Z

@hugorut when I have a change in project with a long name the wrapping looks a bit odd. Should we:

Middle truncate project names like we do with infracost comment?
Or, add forced line wrapping for projects with long names.

aliscott · 2023-05-08T14:24:34Z

Also, should we right-align the Previous and New columns like we do in the Infracost comment?

aliscott · 2023-05-08T14:25:38Z

internal/output/diff.go

+	t.SetStyle(table.StyleBold)
+	t.Style().Format.Header = text.FormatDefault
+	t.AppendHeader(table.Row{
+		"Name",


Suggested change

"Name",

"Project",

To keep it consistent with Infracost comment

aliscott · 2023-05-08T15:23:51Z

internal/output/diff.go

-		s += fmt.Sprintf("\nThe following projects have no cost estimate changes: %s", strings.Join(noDiffProjects, ", "))
-		s += fmt.Sprintf("\nRun the following command to see their breakdown: %s", ui.PrimaryString("infracost breakdown --path=/path/to/code"))
-		s += "\n\n"
+	if hasDiffProjects && out.DiffTotalMonthlyCost != nil && out.DiffTotalMonthlyCost.GreaterThan(decimal.Zero) {


This hides any decreasing costs. I think this should check the absolute value of out.DiffTotalMonthlyCost

Are projects shown based on the same logic as the comment's showProject function?

This hides any decreasing costs. I think this should check the absolute value of out.DiffTotalMonthlyCost

👍

Are projects shown based on the same logic as the comment's showProject function?

No but i'll update so it does

alikhajeh1 · 2023-05-08T15:36:37Z

This is great! A few suggestions:

The colors are confusing as the diff output shows red for - but the table shows green for -. Also, red text might be harder to read. So maybe don't use colors for the summary table?
Add a sentence at the top of the table "Infracost estimate: monthly cost will increase by $X" in the same way we do for the PR comment as that's the ultimate summary line, but without the percentage (as we're going to get rid of the total % from that line). This also clarifies that the table columns are all showing monthly numbers.
Update the numbers in the table to the same logic as the rest of the CLI for decimals, i.e. don't show decimals unless the number is <1 (ROUND_COSTS_ABOVE in the dashboard, not sure what it's called in the CLI code)

hugorut · 2023-05-09T07:08:34Z

Middle truncate project names like we do with infracost comment?

👍

Also, should we right-align the Previous and New columns like we do in the Infracost comment?

👍

* use `showProject` logic to hide/show projects * truncate long project names * add overview cost increase line * remove colours from table * align table columns

hugorut · 2023-05-09T07:49:48Z

@aliscott I've made changes request by you and ali, table looks like this now:

──────────────────────────────────
Infracost estimate: monthly cost will increase by $41 (+100%) ↑

┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━┳━━━━━━━━━━━━┳━━━━━━━━━━━━━━┓
┃ Project                                                          ┃ Previous   ┃ New        ┃ Diff         ┃
┣━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╋━━━━━━━━━━━━╋━━━━━━━━━━━━╋━━━━━━━━━━━━━━┫
┃ infracost/infracost/cmd/infraco...data/terraform_v0.14_plan.json ┃        $41 ┃        $81 ┃ +$41 (+100%) ┃
┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┻━━━━━━━━━━━━┻━━━━━━━━━━━━┻━━━━━━━━━━━━━━┛

I've made the following changes:

Use showProject logic to hide/show projects
Truncated long project names
added overview cost increase line. This contains the % as this is what we use across the CLI and comment, so I kept it consistent so as not to confuse user. When we remove this from the other places (as @alikhajeh1 mentioned) we can remove from here.
Removed colours from table
Right aligned table columns

@alikhajeh1 the table already uses the rounding logic so I'm not sure if you've see something different?

aliscott

❤️ it

alikhajeh1 · 2023-05-09T13:51:00Z

@hugorut re the rounding, it only works on positive numbers, if the diff is <0 then we always seem to show decimals. Can we change that behavior so the absolute value is used to decide?

┃ test                               ┃    $24,773 ┃       $794 ┃ -$23,978.59 (-97%) ┃

hugorut force-pushed the feat/diff-table branch from 627d38f to 8409608 Compare May 8, 2023 12:54

hugorut self-assigned this May 8, 2023

hugorut requested review from aliscott and removed request for aliscott May 8, 2023 13:17

hugorut force-pushed the feat/diff-table branch from 8409608 to 5fcaaf4 Compare May 8, 2023 13:21

hugorut requested a review from aliscott May 8, 2023 13:31

aliscott reviewed May 8, 2023

View reviewed changes

hugorut closed this May 9, 2023

hugorut reopened this May 9, 2023

fix: review changes

042cc1b

* use `showProject` logic to hide/show projects * truncate long project names * add overview cost increase line * remove colours from table * align table columns

aliscott approved these changes May 9, 2023

View reviewed changes

hugorut merged commit 344d310 into master May 9, 2023
10 checks passed

vdmgolub deleted the feat/diff-table branch May 9, 2023 14:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add table output to `diff` #2454

feat: add table output to `diff` #2454

hugorut commented May 8, 2023 •

edited

aliscott commented May 8, 2023

aliscott commented May 8, 2023

aliscott May 8, 2023

aliscott May 8, 2023

hugorut May 9, 2023

alikhajeh1 commented May 8, 2023

hugorut commented May 9, 2023

hugorut commented May 9, 2023

aliscott left a comment

alikhajeh1 commented May 9, 2023

feat: add table output to diff #2454

feat: add table output to diff #2454

Conversation

hugorut commented May 8, 2023 • edited

aliscott commented May 8, 2023

aliscott commented May 8, 2023

aliscott May 8, 2023

Choose a reason for hiding this comment

aliscott May 8, 2023

Choose a reason for hiding this comment

hugorut May 9, 2023

Choose a reason for hiding this comment

alikhajeh1 commented May 8, 2023

hugorut commented May 9, 2023

hugorut commented May 9, 2023

aliscott left a comment

Choose a reason for hiding this comment

alikhajeh1 commented May 9, 2023

feat: add table output to `diff` #2454

feat: add table output to `diff` #2454

hugorut commented May 8, 2023 •

edited