Skip to content
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

use custom winfile scheme to handle windows logger file URLs #977

Merged
merged 9 commits into from
Feb 14, 2019

Conversation

dimroc
Copy link
Contributor

@dimroc dimroc commented Feb 13, 2019

Incorporate fix for windows and file:/// URL incompatibility: uber-go/zap#621

@dimroc dimroc requested a review from jleeh February 13, 2019 16:04
@dimroc dimroc force-pushed the bugs/fix-log-path-for-windows-pt2 branch 2 times, most recently from 4db3634 to 7de9208 Compare February 13, 2019 17:23
@dimroc dimroc force-pushed the bugs/fix-log-path-for-windows-pt2 branch 2 times, most recently from 235652a to 0012977 Compare February 13, 2019 18:16
@dimroc
Copy link
Contributor Author

dimroc commented Feb 13, 2019

@NavyAdmiral could you take a pass for code review and testing on windows?

@dimroc dimroc force-pushed the bugs/fix-log-path-for-windows-pt2 branch from 1c3ba84 to 09589f0 Compare February 13, 2019 20:12
@dimroc dimroc changed the base branch from version-0.6 to master February 13, 2019 20:29
@dimroc dimroc changed the base branch from master to version-0.6 February 13, 2019 20:35
@dimroc dimroc force-pushed the bugs/fix-log-path-for-windows-pt2 branch from 09589f0 to 2c77359 Compare February 13, 2019 20:44
@dimroc dimroc changed the base branch from version-0.6 to master February 13, 2019 20:45
@NavyAdmiral
Copy link
Contributor

Can confirm the fix works. Made sure it was still not working on master and afterwards checked out this PR to see it work, attaching pic just in case:
image
🚢

@jleeh
Copy link
Contributor

jleeh commented Feb 14, 2019

Can confirm this works for me as well 👍

@jleeh
Copy link
Contributor

jleeh commented Feb 14, 2019

I get the same test failure as the PR locally on Windows:

=== RUN   TestClient_LogToDiskOptionDisablesAsExpected
=== PAUSE TestClient_LogToDiskOptionDisablesAsExpected
=== CONT  TestClient_LogToDiskOptionDisablesAsExpected
=== RUN   TestClient_LogToDiskOptionDisablesAsExpected/LogToDisk_=_false_=>_no_log_on_disk
=== RUN   TestClient_LogToDiskOptionDisablesAsExpected/LogToDisk_=_true_=>_log_on_disk_(positive_control)
--- FAIL: TestClient_LogToDiskOptionDisablesAsExpected (0.01s)
    --- FAIL: TestClient_LogToDiskOptionDisablesAsExpected/LogToDisk_=_false_=>_no_log_on_disk (0.00s)
        local_client_test.go:219: 
            	Error Trace:	local_client_test.go:219
            	Error:      	Not equal: 
            	            	expected: false
            	            	actual  : true
            	Test:       	TestClient_LogToDiskOptionDisablesAsExpected/LogToDisk_=_false_=>_no_log_on_disk


Expected :false
Actual   :true
 <Click to see difference>


    --- PASS: TestClient_LogToDiskOptionDisablesAsExpected/LogToDisk_=_true_=>_log_on_disk_(positive_control) (0.00s)
FAIL

Process finished with exit code 1

se3000
se3000 previously approved these changes Feb 14, 2019
it is wrestling with custom URI schemes that shouldn't be known
outside of the package.
@dimroc dimroc force-pushed the bugs/fix-log-path-for-windows-pt2 branch from d4dcb15 to 707c33b Compare February 14, 2019 15:20
@dimroc dimroc merged commit 989dc13 into master Feb 14, 2019
@dimroc dimroc deleted the bugs/fix-log-path-for-windows-pt2 branch February 14, 2019 17:43
asoliman92 pushed a commit that referenced this pull request Jul 31, 2024
## Motivation
In preparation to leader lane fetching inflight gas price, add ability
to fetch all gas price update events as opposed to per chain selector

---------

Co-authored-by: dimkouv <dimitrios.kouveris@smartcontract.com>
asoliman92 pushed a commit that referenced this pull request Jul 31, 2024
## Motivation
In preparation to leader lane fetching inflight gas price, add ability
to fetch all gas price update events as opposed to per chain selector

---------

Co-authored-by: dimkouv <dimitrios.kouveris@smartcontract.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants