-
-
Notifications
You must be signed in to change notification settings - Fork 140
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
Add static analysis to build job #1324
Conversation
Output should look like this on the Github action
|
Thanks for the PR! I made a PR on my fork so I could see the output and it seems to work well. One thing I noticed is for some reason it's not picking up all of the locales. Roku supports 9, we currently support 7, but only 5 are showing up in the static analysis output: As far as this PR, I think we should add the static analysis to both the production build and the dev build workflows so that we know if a PR breaks something. Testing the staging folder makes sense and should match the artifact zip but we may want to test the zip file instead just to verify the zip is what it should be. @neilsb @jimdogx @1hitsong what do you guys think? |
For the locales not detected I think I have the answer. I was digging into this tool and got curious on how it worked. It's an obfuscated java library, that downloads config from http://devtools.web.roku.com/static-channel-analysis/config.json. What's even weirder is that it's an encrypted json file that gets decrypted client side (weird, as that offers no security whatsoever, so I don't know why they did that) Which got me even more curious, so I decrypted the config file, you can find it at https://gist.github.com/iBicha/8b9d542138c0319e1b21038d91cadd13 It's a big json payload with lots of fields related to the checks ran during the analysis. But if you scroll all the way down, you can see there is a |
This pull request has been inactive for 21 days and will be automatically closed in 7 days if there is no further activity. |
This pull request has been closed because it has been inactive for 28 days. You may submit a new pull request if desired. |
Definitely want to add this just haven't had time. The only thing to figure out is whether this should be added to the main build jobs or make a new job for it. |
My first thought is probably a separate job, since there is still some benefit in knowing that the app builds but fails static analysis. But that said, we can't (shouldn't) submit it to the store until the static analysis passes so that may not be a reason for a separate job. |
Yea I think I could argue it either way but I'm leaning on a separate job. For the reason you mentioned and also the added flexibility because we don't have to make the new job a required check on github. That way if roku changes the URL or breaks the tool somehow, our main build jobs won't fail and we'll still be able to merge PRs. |
Changes
@neilsb how's this look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All makes sense, and looks good.
Changes
Add static analysis to build job. This downloads https://devtools.web.roku.com/#static-channel-analysis-tool and analyze the build.
I just added the tool to Playlet, I thought it might save me from unpleasant surprises when I try to publish.
Things to consider:
https://devtools.web.roku.com/static-channel-analysis/sca-cmd.zip
without a pinned version, let's hope that Roku folks are good at not breaking thingsThat being said, feel free to modify, merge, repurpose, or close this PR
Issues
N/A