-
Notifications
You must be signed in to change notification settings - Fork 0
Security groups request ids #1
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
base: master
Are you sure you want to change the base?
Conversation
The python api will now output a table with requestIDs for specific calls to security groups apis
Fix unit tests Fix code style issues
Add new return format
Add functionality to get event logs to slcli
Fix Security Group unit tests
Refactor Event Log Code Fix Unit Tests Add Unit Tests Fix Code Styling
SoftLayer/managers/event_log.py
Outdated
| @@ -0,0 +1,28 @@ | |||
| """ | |||
| SoftLayer.network | |||
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 think the pkg name is wrong
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.
Fixed.
SoftLayer/managers/network.py
Outdated
| 'virtualGuests', | ||
| ]) | ||
|
|
||
| CCI_SECURITY_GROUP_EVENT_NAMES = [ |
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 this used?
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.
Nope. Removed.
SoftLayer/CLI/event_log/get.py
Outdated
| request_filter['objectName'] = {'operation': obj_type} | ||
|
|
||
| return filter | ||
| return request_filter |
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.
In the GUI, you can also filter by Action.
Is it possible to also filter by request id? I suppose since this CLI is not specific to security groups, that would mean a generic metadata filter is needed.
The GUI also has DateFrom and DateTo filters. That would be another way to limit the output.
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'd have to see how those filters are built for calls to Event_Log.getAllObjects
Remove unneeded code left over from refactoring Fix incorrect package name
Code Styling changes
tartley
left a comment
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.
Some minor comments and questions, but overall looks great to me.
|
|
||
| for log in logs: | ||
| try: | ||
| metadata = json.dumps(json.loads(log['metaData']), indent=4, sort_keys=True) |
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 like the key sorting, good idea.
| args=([{'id': '520', | ||
| 'direction': 'ingress'}],)) | ||
|
|
||
| self.assertEqual([{'requestId': 'editRules'}], json.loads(result.output)) |
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 like that you use a real data structure (not a string) for the 'actual', and hence do a json.loads on the 'expected'. Helps to guard against typos and invalid data, and keeps the test working even if the output changes in semantically transparent ways like whitespace.
tests/CLI/modules/event_log_tests.py
Outdated
| } | ||
| ] | ||
|
|
||
| self.assertEqual(correctResponse, json.loads(result.output)) |
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.
In general, I'm used to a Python convention for test assertions args being 'actual' followed by 'expected', to help disambiguate in fiddly cases. (The mnemonic being 'a' comes before 'e'.) i.e:
self.assertEqual(json.loads(result.output), correctResponse)
Is the convention the reverse in this repo?
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.
It flip flops. I'll go with Actual then Expected though. Makes sense.
Change public facing name to Audit Logs Add functionality to get event log types
Fix ordering of test expecations to be Actual then Expected
Add functionality to filter by eventName
Add functionality to filter by dates
Add request id search functionality
Fix fixtures from merge
Fix tox issues
change date_min to date-min in click args change date_max to date-max in click args change how the event log client is initialized move filter building code into event log manager set default utc offset to +0000 move date parsing code into utils add ability to get event logs by type add ability to get event logs by name move requestID searching into Security Groups code Overhaul unit tests
No description provided.