karlderkaefer / cdk-notifier

CLI tool to post AWS CDK diff as comment to Github pull request
MIT License
122 stars 7 forks source link

feat: Don't truncate PR comments #146

Closed joshmyers closed 7 months ago

joshmyers commented 8 months ago

What

Terraform refugee here :wave: - thanks for the tool!

Atlantis style PR comments that are not truncated but roll over onto new comments prefixed/suffixed to let the reviewer know there are continuations.

WIP PR (tests to be addressed/added) to gauge interest in this feature? Implementation could be cleaned up a bit too. Open to suggestions.

Why

So PR comments aren't truncated potentially missing changes.

Testing

I have tested this against GHE, see below. Ironically it seems that GHE can set arbitrary(??) comment limits and while I don't know what ours currently is, it is significantly more than 65536 that github.com sets as the entire comment below could fit into a single comment. Perhaps these could be config rather than hard coded consts.

image

Example continuation:

image

karlderkaefer commented 8 months ago

I appreciate your effort to implement this. However this is increasing the complexity of the code. If possible I would like to keep it simple. I would suggest if the diff is too long, we can just provide a link to the CI Job instead (forgot to implement this feature). The Job Link should contain the link to the CI job and the user can view full diff log there https://github.com/karlderkaefer/cdk-notifier/blob/1dd6d8b2a00b262af868d94bc446afbfe0486691/transform/template.go#L74

on time of writing the maximum length for PR and issues on github was 65536. This might have been fixed or improved now. We need to double check the limits again for each VCS. Maybe we could just increase further, that would already help with your issue I guess.

The code is quite ok, but just from user perspective I would find it confusing to split over multiple comments, especially if you have a multi stack environment. I would rather add rather append a link to CI Job log output to the truncate message and to the header message of the comment

joshmyers commented 8 months ago

Hey @karlderkaefer - thanks for the quick reply. That is fair enough, it does bring some extra complexity to the code base and the naive implementation currently relies on things like the list being returned in order. I think your alternative solution could also work.

Re limits, github.com is still 65536 but GHE can be more, as ours apparently is. If this was configurable vs constant, we could continue to use the tool, not get truncated messages (well, less likely) and not require the comment continuations. WDYT? For reasons out of my control and somewhat down to CDK/CFN hacks applied, we by default have larger than ideal diffs so often see truncated.

karlderkaefer commented 8 months ago

@joshmyers Good idea. I would appreciate if you can implement the increase of maximum comment per VCS length like you already did. Just removing the comment splitting. Adding a parameter to overwrite maximum for github enterprise also makes sense to me. I will implement mean while the job link

karlderkaefer commented 8 months ago

I added job link https://github.com/karlderkaefer/cdk-notifier/pull/148

joshmyers commented 8 months ago

Sure, I'll try and get to it this week. Thanks.

joshmyers commented 7 months ago

See https://github.com/karlderkaefer/cdk-notifier/pull/154