search

pulumi / actions

Deploy continuously to your cloud of choice, using your favorite language, Pulumi, and GitHub!
Apache License 2.0
258 stars 72 forks source link

Trim PR comments from the front. #1252

Closed devonmoss closed 2 months ago

devonmoss commented 3 months ago

I believe that the resources summary is the most important component of the output provided by pulumi preview and should always be included in the pull request comment. When trimming from the back, long outputs will not contain this information, which requires navigating to the checks tab and looking at the output there.

Perhaps this is just a reflection of how I use pulumi but don't think so because we are all conditioned by the local use of the pulumi cli. When we run pulumi preview locally the output ends up right at the bottom so the first thing we see is the summary something like this:

Resources:
      ~ 85 to update
      +-26 to replace
      111 changes. 1062 unchanged

In many cases this is all the information that is needed to confirm that I'm ready to run pulumi up. Other times I need to look more closely at the details for each resource but even still I have the information from that summary to give me an idea of what I'm looking at as I scroll back through the output.

This change trims the PR comment from the front if it is ever so long that it must be trimmed.

tgummerer commented 3 months ago

Thanks for the contribution. I'm not sure I agree that this is better than what we have currently though. Could you give us a bit of a description of why you think this is better? (This should also be included in the PR description) Maybe we could make this behaviour optional and add another flag to toggle it? Trimming from the front of the message seems unconventional and should probably not be the default behaviour.

devonmoss commented 3 months ago

I've updated the description for my PR.

I agree that generally speaking, trimming from the front is unconventional but in this case it causes a scenario where the resources summary is not shown in the pull request comment and I can't see why this would ever be a desirable situation.

voldemortensen commented 2 months ago

I agree with @devonmoss here. When I'm reviewing potential changes, I am much less interested in the output of the pip install and much more interested in which resources are going to be effected by a merge request. In fact, unless there's an error with pip install, I'm not really interested in that output at all.

devonmoss commented 2 months ago

I've added a commit which allows this to be configured optionally. My apologies for not reading the CONTRIBUTING.md file before submitting. I see now that I should have submitted an issue first. I'll be better next time.

pulumi-bot commented 2 months ago

This PR has been shipped in release v5.5.0.