-
Notifications
You must be signed in to change notification settings - Fork 966
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
Log full request path and responses in dry-run.rb #6459
Log full request path and responses in dry-run.rb #6459
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.
This looks like a nice improvement, thanks!
I think we may want to hold this PR for a bit until we're sure we're ready to jump to the most recent excon version since we had to pin ourselves to 0.93.1 recently. See 6d59a5a.
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 all you really need here is the dry-run.rb
change? The gemspec
/ Gemfile.lock
aren't needed for this PR IIUC??
How about the changes in excon/excon#803 without which the response doesn't have crucial aspects of the URI? |
Ah, that's why you're bumping the minimum version... makes sense. I was only thinking about how it will default to the max version. |
Can you fix merge conflicts so we can merge? |
Done |
Thanks again! |
When executing
dry-run.rb
script, it would be ideal to log the full URL (built usingExcon::Utils.request_uri(payload)
) for requests that share the path but different query parameters, especially in Azure DevOps.Logging the response is also useful when debugging.
This is a result of excon/excon#803 hence the bump in excon version
Sample log with this change:
When a requests return 404, we can check if the path is correct or where an issue could be. In scenarios where the body is necessary,
EXCON_DEBUG=1
can be used.