-
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
Conversation
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.
Can we add a couple of log unit tests as well? Thanks!
headers = {} | ||
headers['Content-Type'] = 'application/json' | ||
headers['If-Modified-Since'] = @last_modified if @last_modified | ||
headers['Authorization'] = "Bearer #{@access_token}" unless @access_token.nil? |
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 Authorization
header 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
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']] |
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.
nit: what happens if last modified is nil?
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 is assigned as nil. I just moved this line of code inside the check. assuming it was working correctly earlier as well without a nil
check
headers = {} | ||
headers['Content-Type'] = 'application/json' | ||
headers['If-Modified-Since'] = @last_modified if @last_modified | ||
headers['Authorization'] = "Bearer #{@access_token}" unless @access_token.nil? |
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 Authorization
header 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.
Oops. I saw that you were outputting *** after closer inspection. Sorry about that. But, can we add a quick log test to the http_project_config_manager_spec?
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.
LGTM
Summary
Added more debug logs to datafile manager to make debugging easy.
Test plan
All Existing unit and FSC tests should pass