Skip to content

feat: Add flag to strip refresh output from errored plans #5448

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

patrickvinograd
Copy link
Contributor

what

  • Adds a server config flag "strip-refresh-output-from-errors"
  • If true, strips the "Refreshing state..." lines from plan error output, as it does for successful plan output.

why

  • With large states, any error during planning results in a flurry of Github comments (and in our case, slack messages) containing all the "Refreshing state..." messages. We find these are rarely any help in resolving the plan error.

  • But, some people may want the full output, so make it a configurable flag to allow choice in the matter.

  • I considered making this the default and controlling it with the Verbose flag, but wasn't sure if overriding that flag's semantics was wise.

  • I also considered returning the full vs. stripped output in the result object, and allowing users to customize what gets rendered via templates/template overrides. But this might involve changes to existing templates/variables and thus break people's overrides in the wild.

tests

  • Added a unit test in plan_step_runner_test.go

references

@patrickvinograd patrickvinograd requested review from a team as code owners March 26, 2025 19:57
@patrickvinograd patrickvinograd requested review from chenrui333, lukemassa and X-Guardian and removed request for a team March 26, 2025 19:57
@dosubot dosubot bot added feature New functionality/enhancement go Pull requests that update Go code labels Mar 26, 2025
@github-actions github-actions bot added the docs Documentation label Mar 26, 2025
@patrickvinograd patrickvinograd force-pushed the pv-strip-refresh-output-errors branch from 994457f to c26781c Compare March 26, 2025 20:03
@jamengual
Copy link
Contributor

hi @patrickvinograd can you just do this creating your own templates?

@patrickvinograd
Copy link
Contributor Author

hi @patrickvinograd can you just do this creating your own templates?

Possibly, I mentioned that option in the PR comment. But as of now wrappedErr.tmpl just renders .Error. I would need to plumb the stripped version of the terraform output through command.ProjectResult to make it available as a template variable. Then do I also include the full output and the stripped output, plus it's kind of duplicative of ProjectResult.PlanSuccess.TerraformOutput, but that's not even set on an error. Enough uncertainty there that I opted to keep the stripping logic in the plan_step_runner, just controlled by a config flag.

I do agree there's already a ton of config flags and templates are a nice solution. Happy to re-work, I just would appreciate some guidance about where in ProjectResult it would be best to plumb these result variations through.

@jamengual
Copy link
Contributor

I will ask @lukemassa @chenrui333 to chime in.
I will say that extending the templates will be better since that will allow further trimming as needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation feature New functionality/enhancement go Pull requests that update Go code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants