Skip to content

Cost Estimation CLI output unnecessary#27735

Merged
omarismail merged 2 commits intohashicorp:masterfrom
omarismail:cost-estimation-output
Feb 11, 2021
Merged

Cost Estimation CLI output unnecessary#27735
omarismail merged 2 commits intohashicorp:masterfrom
omarismail:cost-estimation-output

Conversation

@omarismail
Copy link
Contributor

@omarismail omarismail commented Feb 10, 2021

Problem

When an error occurs during terraform apply, the "Cost Estimation" output appears, even though it shouldn't.

See Screenshot

image

Solution

There is nothing wrong with the specific conditions in the remote backend for cost estimations. Rather, the CLI.Output should appear further below depending on the Status returned from the api/v2/cost-estimation endpoint in the remote backend.

image

@hashicorp-cla
Copy link

hashicorp-cla commented Feb 10, 2021

CLA assistant check
All committers have signed the CLA.

}

if b.CLI != nil {
b.CLI.Output("\n------------------------------------------------------------------------\n")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved the starting output ---- here because on line 274 we check to ensure that there are no errors/cancellations.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean that we might see a couple of lines of ----s if we hit CostEstimatePending, tfe.CostEstimateQueued? I know this kind of procedural flow makes it hard to do this correctly, but I'm thinking we shouldn't show this separator every iteration

deltaRepr := strings.Replace(ce.DeltaMonthlyCost, "-", "", 1)

if b.CLI != nil {
b.CLI.Output(b.Colorize().Color(msgPrefix + ":\n"))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved the specific msgPrefix part here and the other lines below because other cases have a different prefex mesage, and there wasn't a clean way to not duplicate.

@omarismail omarismail marked this pull request as ready for review February 10, 2021 16:25
@omarismail omarismail requested review from alisdair, chrisarcand, nfagerlund, radditude and thrashr888 and removed request for radditude and thrashr888 February 10, 2021 16:25
@codecov
Copy link

codecov bot commented Feb 10, 2021

Codecov Report

Merging #27735 (4ac095b) into master (0a99757) will increase coverage by 0.00%.
The diff coverage is 80.00%.

Impacted Files Coverage Δ
backend/remote/backend_common.go 50.35% <80.00%> (-0.19%) ⬇️
terraform/evaluate.go 52.89% <0.00%> (+0.41%) ⬆️
dag/marshal.go 63.49% <0.00%> (+1.58%) ⬆️
terraform/node_resource_plan.go 98.05% <0.00%> (+1.94%) ⬆️

Copy link
Contributor

@mwhooker mwhooker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this, @omarismail! I like the changes, but I think figuring out a way to avoid sending the horizontal-rule every retry would be preferable. Unfortunately you might have to copy & paste the same way as you did with msgPrefix.

@omarismail
Copy link
Contributor Author

Thanks for fixing this, @omarismail! I like the changes, but I think figuring out a way to avoid sending the horizontal-rule every retry would be preferable. Unfortunately you might have to copy & paste the same way as you did with msgPrefix.

I changed the first horizontal rule to only run on the first attempt.

Copy link
Contributor

@mwhooker mwhooker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@omarismail omarismail merged commit 14936e6 into hashicorp:master Feb 11, 2021
@omarismail omarismail deleted the cost-estimation-output branch February 11, 2021 13:20
@ghost
Copy link

ghost commented Mar 14, 2021

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked as resolved and limited conversation to collaborators Mar 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants