search

18F / concourse-compliance-testing

Concourse CI assets for Compliance Toolkit
https://compliance-viewer.18f.gov/
Other
17 stars 7 forks source link

ZAP Summary #26

Closed ctro closed 8 years ago

ctro commented 8 years ago

Ref #24

Working locally for me. Big changes since this was all checked into scripts:

I feel better about this now, but I'm still failing the codeClimate build, though much less egregiously. It seems to me that lots of the warnings would go away if I wrote this Classy/Statefully instead of Functionally/Statelessly, which I can do if needed... But I'm pretty happy with the simplicity/readability of this PR vs the stuff that was in the script directory.
Apologies for essentially rewriting all of this, but the functions are pretty much identical.

Also are we going to work under a 'hard' CodeClimate rating/limit? i.e. do we only merge if it's a 4.0?

ctro commented 8 years ago

@afeld @DavidEBest, I think this is in a much better place now. Review! ? :) It should just work if you pull down the branch and run it.

DavidEBest commented 8 years ago

This seems pretty close. Could you output the summary.txt content into the slack message? Should be just replacing the text field with text_file pointing to summary.txt. https://github.com/cloudfoundry-community/slack-notification-resource

Additionally, could you upload the json version of the summary to s3?

ctro commented 8 years ago

@DavidEBest summary.txt -> Slack. summary.json -> S3. I had to create another instance of the s3-resource-simple -- the options only work when attached to a Resource. I'll be out the rest of today(Friday). If there are any other issues here I'll take care of them later tonight else the weekend. THANKS!

afeld commented 8 years ago

Also, feel free to disable Feature Envy in reek if you disagree with those warnings in Code Climate.

ctro commented 8 years ago

I think reek/Feature Envy is still useful. I like seeing the recommendations even if I don't always agree that they make code simpler/better/easier-to-understand.

DavidEBest commented 8 years ago

I've not used reek before, but that output seems a bit intense. Is limiting methods to 5 statements a bit too strict?

This looks good, and is working for me locally.