-
Notifications
You must be signed in to change notification settings - Fork 28
refactor: Added more logs to datafile manager #260
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -148,16 +148,18 @@ def fetch_datafile_config | |
| end | ||
|
|
||
| def request_config | ||
| @logger.log( | ||
| Logger::DEBUG, | ||
| "Fetching datafile from #{@datafile_url}" | ||
| ) | ||
| begin | ||
| headers = {} | ||
| headers['Content-Type'] = 'application/json' | ||
| headers['If-Modified-Since'] = @last_modified if @last_modified | ||
| headers['Authorization'] = "Bearer #{@access_token}" unless @access_token.nil? | ||
| @logger.log(Logger::DEBUG, "Fetching datafile from #{@datafile_url}") | ||
| headers = {} | ||
| headers['Content-Type'] = 'application/json' | ||
| headers['If-Modified-Since'] = @last_modified if @last_modified | ||
| headers['Authorization'] = "Bearer #{@access_token}" unless @access_token.nil? | ||
|
|
||
| # Cleaning headers before logging to avoid exposing authorization token | ||
| cleansed_headers = {} | ||
| headers.each { |key, value| cleansed_headers[key] = key == 'Authorization' ? '********' : value } | ||
| @logger.log(Logger::DEBUG, "Datafile request headers: #{cleansed_headers}") | ||
|
|
||
| begin | ||
| response = Helpers::HttpUtils.make_request( | ||
| @datafile_url, :get, nil, headers, Helpers::Constants::CONFIG_MANAGER['REQUEST_TIMEOUT'] | ||
| ) | ||
|
|
@@ -169,6 +171,9 @@ def request_config | |
| return nil | ||
| end | ||
|
|
||
| response_code = response.code.to_i | ||
| @logger.log(Logger::DEBUG, "Datafile response status code #{response_code}") | ||
|
|
||
| # Leave datafile and config unchanged if it has not been modified. | ||
| if response.code == '304' | ||
| @logger.log( | ||
|
|
@@ -178,9 +183,14 @@ def request_config | |
| return | ||
| end | ||
|
|
||
| @last_modified = response[Helpers::Constants::HTTP_HEADERS['LAST_MODIFIED']] | ||
|
|
||
| config = DatafileProjectConfig.create(response.body, @logger, @error_handler, @skip_json_validation) if response.body | ||
| if response_code >= 200 && response_code < 400 | ||
| @logger.log(Logger::DEBUG, 'Successfully fetched datafile, generating Project config') | ||
| config = DatafileProjectConfig.create(response.body, @logger, @error_handler, @skip_json_validation) | ||
| @last_modified = response[Helpers::Constants::HTTP_HEADERS['LAST_MODIFIED']] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: what happens if last modified is nil?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is assigned as nil. I just moved this line of code inside the check. assuming it was working correctly earlier as well without a |
||
| @logger.log(Logger::DEBUG, "Saved last modified header value from response: #{@last_modified}.") | ||
| else | ||
| @logger.log(Logger::DEBUG, "Datafile fetch failed, status: #{response.code}, message: #{response.message}") | ||
| end | ||
|
|
||
| config | ||
| end | ||
|
|
||
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.
You are right, that is exactly what i did. This is why i cleansed the headers before logging and replaced the value of
Authorizationheader to***right below this line.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.
ha, i saw that. i deleted my comment. but, it would be nice to add a simple logger spy test to see that was outputted :)
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.
yup.. good idea.. doing that now.
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.
Added tests