Conversation
kimburgess
left a comment
kimburgess
left a comment
There was a problem hiding this comment.
Perf comments on old office lib
| data = data.to_json if !data.nil? && data.class != String | ||
|
|
||
| if password | ||
| headers['Authorization'] = "Bearer #{password_graph_token}" |
There was a problem hiding this comment.
This performs an OAuth token request (possible secondary point of rate limiting) prior to the actual graph request taking place. Caching tokens should help reduce request overhead / number of round trips needed.
| if password | ||
| headers['Authorization'] = "Bearer #{password_graph_token}" | ||
| else | ||
| headers['Authorization'] = "Bearer #{graph_token}" |
| graph_api = UV::HttpEndpoint.new(@graph_domain, graph_api_options) | ||
| response = graph_api.__send__(request_method, path: graph_path, headers: headers, body: data, query: query) | ||
|
|
||
| start_timing = Time.now.to_i |
There was a problem hiding this comment.
This is not used anywhere (possible left over from some debug logging)?
|
|
||
| start_timing = Time.now.to_i | ||
| response_value = response.value | ||
| end_timing = Time.now.to_i |
|
|
||
| def check_response(response) | ||
| case response.status | ||
| when 200, 201, 204 |
There was a problem hiding this comment.
Using a range for 200-ish response (e.g. when 200..299) will catch all generally ok responses.
| raise Microsoft::Error::ErrorAccessDenied.new(response.body) | ||
| when 404 | ||
| raise Microsoft::Error::ResourceNotFound.new(response.body) | ||
| end |
There was a problem hiding this comment.
Graph API returns 429's in the case of rate limiting. It's worth handling that explicitly here.
| when 404 | ||
| raise Microsoft::Error::ResourceNotFound.new(response.body) | ||
| end | ||
| end |
There was a problem hiding this comment.
All other response codes are currently silently swallowed. Worth adding an else condition for these.
| endpoint = "/v1.0/users" | ||
| request = graph_request(request_method: 'get', endpoint: endpoint, query: query_params, password: @delegated) | ||
| check_response(request) | ||
| user_list = JSON.parse(request.body)['value'] |
There was a problem hiding this comment.
Depending on response sizes, it may be worth handing this off to a threadpool.
There was a problem hiding this comment.
Ditto for all other occurrences of JSON.parse
| '$filter': "createdDateTime gt #{created_from}" | ||
| } | ||
|
|
||
| bulk_response = bulk_graph_request(request_method: 'get', endpoints: endpoints, query: query ) |
There was a problem hiding this comment.
This call is synchronous (due to co-routine / awaiting response). This may be intended, but if not, this slicing / loop could be refactored to query each slice in parallel.
|
|
||
| event = event.to_json | ||
|
|
||
| event.gsub!("X!X!X!",description) |
There was a problem hiding this comment.
Can this be inserted directly into attributes that need the substitution before stringifying to JSON? Should cut down the amount of string manipulation needed.
… Ignore power query response (it's always 'false')
chore(pexip) update system_location
Attempt to resolve build error:
```
Bundler could not find compatible versions for gem "ruby":
In Gemfile:
ruby
aca-device-modules was resolved to 2.0.0, which depends on
ruby (>= 2.3.0)
aca-device-modules was resolved to 2.0.0, which depends on
orchestrator was resolved to 2.0.0, which depends on
rails (~> 6.0) was resolved to 6.1.7.8, which depends on
ruby (>= 2.5.0)
```
9 participants
Merges the 'working copy' of this repo back into master in order to consolidate development ahead of implementation on new simplified branching structure and review process.
Currently awaiting merge of other active changes prior to review.