Skip to content
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

Remove mention of security issue when concerning insight is removed #187

Merged

Conversation

@Jibbarth
Copy link
Collaborator

commented Jun 17, 2019

Q A
Bug fix? yes
New feature? no
Fixed tickets #181

This remove mention of security issues found when we remove ForbiddenSecurityIssues Insight.

I don't know if it's the better way. I'm not huge fan of the new public function hasInsightInCategory in Results, but I wanted to leave the function getTotalSecurityIssues returning 0 even if the Insight is removed to not alter the AnalyseCommand.

WDYT ?

@olivernybroe

This comment has been minimized.

Copy link
Collaborator

commented Jun 17, 2019

What about showing a message about it being disabled? This way we don't show a confusing 0 message or no message at all.

@nunomaduro nunomaduro merged commit 7121128 into nunomaduro:master Jun 18, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nunomaduro

This comment has been minimized.

Copy link
Owner

commented Jun 18, 2019

Thanks for this.

@nunomaduro

This comment has been minimized.

Copy link
Owner

commented Jun 18, 2019

@olivernybroe Just saw your message, I think it's a good idea remove it from the console output for now.

@Jibbarth Jibbarth deleted the Jibbarth:fix/issue-181-remove-security-mention branch Jun 18, 2019

@Jibbarth Jibbarth referenced this pull request Jun 20, 2019

Closed

[internal] Release 1.7.0 ? #191

7 of 7 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.