-
Notifications
You must be signed in to change notification settings - Fork 276
Conversation
blueandgold
commented
Oct 23, 2018
•
edited
Loading
edited
- Also maintains backward compatibility with the alpha API. If you can see a better way to do this, please let me know.
- Will update tests after review.
|
||
client = securitycenter.SecurityCenterClient(version='v1beta1') | ||
|
||
for i in findings: |
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.
finding/finding_tuple would be easier to understand than i here e.g.
for finding in findings
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.
Good suggestion, done.
client = securitycenter.SecurityCenterClient(version='v1beta1') | ||
|
||
for i in findings: | ||
finding_id = i[0][0] |
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.
Should this be i[0] instead?
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.
Good catch, this was sliced incorrectly, and is now fixed.
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.
Thanks for the review. Please take another look.
|
||
client = securitycenter.SecurityCenterClient(version='v1beta1') | ||
|
||
for i in findings: |
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.
Good suggestion, done.
client = securitycenter.SecurityCenterClient(version='v1beta1') | ||
|
||
for i in findings: | ||
finding_id = i[0][0] |
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.
Good catch, this was sliced incorrectly, and is now fixed.
@@ -77,8 +87,17 @@ def __init__(self, **kwargs): | |||
|
|||
LOGGER.debug( | |||
'Creating _SecurityCenterOrganizationsFindingsRepositoryClient') | |||
|
|||
# pylint: disable=protected-access | |||
if kwargs.get('gcp_service')._resourceDesc.get('version') == 'v1beta1': |
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.
Is it possible for 'gcp_service' to be not found? If so accessing _resourceDesc will throw errors (Can't seem to find any information related to _resourceDesc, too).
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.
gcp_sevice
is always built by the create_service_api()
in _base_repository()
.
# CSCC can't accept the full hash, so this must be shortened. | ||
finding_id = violation.get('violation_hash')[:32] | ||
finding = { | ||
'name': '{0}/findings/{1}'.format( |
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.
Any character limits on any of these fields?
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.
I believe that the character limits are actually bigger than on alpha api. Let me follow-up with the CSCC team to be sure.
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.
Thanks for the review. All comments addressed.
# CSCC can't accept the full hash, so this must be shortened. | ||
finding_id = violation.get('violation_hash')[:32] | ||
finding = { | ||
'name': '{0}/findings/{1}'.format( |
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.
I believe that the character limits are actually bigger than on alpha api. Let me follow-up with the CSCC team to be sure.
@@ -77,8 +87,17 @@ def __init__(self, **kwargs): | |||
|
|||
LOGGER.debug( | |||
'Creating _SecurityCenterOrganizationsFindingsRepositoryClient') | |||
|
|||
# pylint: disable=protected-access | |||
if kwargs.get('gcp_service')._resourceDesc.get('version') == 'v1beta1': |
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.
gcp_sevice
is always built by the create_service_api()
in _base_repository()
.
…ti-security into csccbeta_final
Codecov Report
@@ Coverage Diff @@
## dev #2130 +/- ##
==========================================
- Coverage 88.88% 88.78% -0.11%
==========================================
Files 175 175
Lines 13462 13515 +53
==========================================
+ Hits 11966 11999 +33
- Misses 1496 1516 +20
|
* Add support for CSCC Beta API * tweak * tweak * address comments and fix tests * update test for beta api * add more tests for beta api * fix tests * fix lint