Daniel15
Configuration option to mark SHA status as non-success
I'm trying this out for angular/angular which has a clunky brittle size-limit test.
I see from https://github.com/alexeagle/angular-bazel-example/pull/74 that I get a nice comment from the bot. But I also want the green merge button to go away, and force whoever merges the commit to think about it. I'd like to do this by marking that commit SHA with a "pending" status. Thoughts about whether such a feature belongs?
err, sorry. it does add one, but it's green I think this is what I want https://github.com/Daniel15/BuildSize/blob/d5f12fc661b2236270158cc86ad7d23543b33ed1/app/Listeners/GitHubStatusListener.php#L38
It was commented out in this commit https://github.com/Daniel15/BuildSize/commit/659e6fea871265347d5d757cd8f8ea186fc4d7b9#diff-ec5c124326cf93ffb0186797120d22edR234
@Daniel15 do you remember why?
Answer from another thread: it was disabled until it can be configurable.
For reference, https://github.com/angular/angular/pull/22041 should have had a red status.
Another detail here: https://github.com/angular/angular/pull/22056 introduced a new artifact. The logic is to add the sizes, thus this means an overall size increase of the artifacts stored. In this issue, we'd make the status of this PR red. I don't think a PR that measures a new artifact should be red, and I don't think total is the right measure - I'd rather take the max(deltas(artifacts)) - that is, for artifacts that have a delta (new ones wouldn't), report on whichever one grew the most.
Here's an example of the opposite happening. https://github.com/angular/angular/pull/21939#issuecomment-365058011 While the apparent total size is decreasing, that's just because we are no longer tracking that file, due to it being deleted.
Also I just noticed that there are two different numbers on that issue for how much the build is changing by, even though the delta number remains the same (2.77KB)
@Toxicable It makes sense in some cases (in that deleting one file does reduce the total size of build artifacts). I agree that it's a bit confusing though.
I'd rather take the max(deltas(artifacts)) - that is, for artifacts that have a delta (new ones wouldn't), report on whichever one grew the most.
This is a good idea! Would you like to submit a PR for it? If not, I could try to take a look once I get some free time :)
Yes I'm working with @Toxicable on all the issues/FRs I filed, we're happy to contribute
On Tue, Feb 13, 2018, 10:15 PM Daniel Lo Nigro [email protected] wrote:
@Toxicable https://github.com/toxicable It makes sense in some cases (in that deleting one file does reduce the total size of build artifacts). I agree that it's a bit confusing though.
I'd rather take the max(deltas(artifacts)) - that is, for artifacts that have a delta (new ones wouldn't), report on whichever one grew the most.
This is a good idea! Would you like to submit a PR for it? If not, I could try to take a look once I get some free time :)
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Daniel15/BuildSize/issues/31#issuecomment-365507175, or mute the thread https://github.com/notifications/unsubscribe-auth/AAC5I6IBdEU0xJhZ0oQdl380x2pg4EPdks5tUnnigaJpZM4R09df .
So just to be clear. Making this change will change both how the comment and status are displayed. Out of all the files change it will find the max change and report that + it's file name Here is what I think it should look like:
Status: Decrease: main.bundle.js decreased by 2.77 KB (25%) Increase: main.bundle.js increased by 2.77 KB (25%) No Change: No changes
Comment: I think the table should remain the same, but remove the text part of it since that's just reiterating the status and can be read off the table
Out of all the files change it will find the max change and report that + it's file name
This sounds like a great idea!