-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Make bloat check work with github actions #1441
Conversation
… fix a LGTM.com warning ... not totally useless!
…que and easy to detect
…ough- we should use the same docker image version
Another idea would be for PRs to upload their binaries as usual and have the bloat check run in the scheduled process (we delete the outputs when bloats are generated). This would have the benefit of easily figuring out if a bloat report already ran or not. |
I think @jwhui and the OpenThread team has some of these problems addressed. Are there solutions we can leverage there? |
I believe the solution there was a github app - I had the link when we previously discussed this. |
https://github.com/apps/size-report/ was mentioned in #541 |
Docs are a bit light there. I believe I currently know what to do to make our bloaty-based reports work, however I cannot estimate what it takes to get size-report to work. I would prefer if someone else more familiar tries for the size-report integration and then we can compare (or if one approach seems a time sink, we should just keep with whatever we get working first) |
@andy31415 could you install the size-report app on this project, so that I can try bringing it up on this project? |
I don't think I have sufficient rights. @woody-apple: can we install the size-report app? |
Separately, for this PR in particular: @saurabhst @jelderton @robszewczyk ? |
Done! |
@andy31415 can you sign the CLA? we can merge then :) |
This pull request fixes 2 alerts when merging dbc5cb7 into efa630b - view on LGTM.com fixed alerts:
|
dbc5cb7
to
4351126
Compare
This pull request fixes 2 alerts when merging 4351126 into ace290d - view on LGTM.com fixed alerts:
|
This will apparently require some iterations to fix - still unclear to me why builds fail since this was supposed to be a python only change. |
This pull request fixes 2 alerts when merging 59daba0 into ace290d - view on LGTM.com fixed alerts:
|
Problem
We moved from CircleCI to github (which supports IPv6). Need to update bloat check since APIs are probably different.
Summary of Changes
Exploring changes needed to get bloat checks to work again