8000 Fixups for 39695 - edit comments, redundant asserts by vikramhh · Pull Request #39752 · moby/moby · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fixups for 39695 - edit comments, redundant asserts #39752

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 17, 2019
Merged

Fixups for 39695 - edit comments, redundant asserts #39752

merged 1 commit into from
Aug 17, 2019

Conversation

vikramhh
Copy link
@vikramhh vikramhh commented Aug 15, 2019 •
edited by thaJeztah
Loading

follow-up to #39695

  1. Modify comments added in 5858a99 (Builder: fix "COPY --from" to non-existing directory on Windows #39695)
    Windows Volume GUID path format is: \?\Volume{}<path>
    Rewrote the example given in comments to conform to the format..

  2. Remove two redundant asserts[assert.NilError]. They are redundant
    because the last statement will not change the value of err.

Signed-off-by: Vikram bir Singh vikrambir.singh@docker.com

- What I did
Modified comments and redundant asserts.

- How I did it
Edited comments, removed the redundant asserts.

- How to verify it
Code inspection is the best way to verify these changes.
The only other way that I can think of verifying the redundant asserts is to
create an environment where they would be fired and verify they appear twice as failures.

- Description for the changelog
Fixups for 39695 - edit comments, redundant asserts

- A picture of a cute animal (not mandatory but encouraged)

8000

@vikramhh vikramhh requested a review from tonistiigi as a code owner August 15, 2019 19:01
@vikramhh
Copy link
Author

@ddebroy PTAL

@ddebroy
Copy link
Contributor
ddebroy commented Aug 15, 2019

LGTM
/cc @thaJeztah

@vikramhh
Copy link
Author
vikramhh commented Aug 15, 2019 •
edited
Loading

I am hitting exactly the same windowsRS1 failure in #39748 which does not have these changes. It seems that the particular test is flaky. I do see some instances of that particular error message being discussed in this issue: #36218

13:04:12 ----------------------------------------------------------------------
13:04:12 FAIL: check_test.go:107: DockerSuite.TearDownTest
13:04:12 
13:04:12 assertion failed: error is not nil: Error response from daemon: container c85ca051156d6bab77f77909aeaf03047571b238a5032e775575a517d3c4bb54: driver "windowsfilter" failed to remove root filesystem: failed to detach VHD: The device is not ready.: rename D:\CI\CI-7f157e2c61\daemon\windowsfilter\c85ca051156d6bab77f77909aeaf03047571b238a5032e775575a517d3c4bb54 D:\CI\CI-7f157e2c61\daemon\windowsfilter\c85ca051156d6bab77f77909aeaf03047571b238a5032e775575a517d3c4bb54-removing: Access is denied.: failed to remove c85ca051156d6bab77f77909aeaf03
8000
047571b238a5032e775575a517d3c4bb54
13:04:12 
13:04:12 ----------------------------------------------------------------------
13:04:12 PANIC: docker_cli_run_test.go:4152: DockerSuite.TestRunCredentialSpecFailures
13:04:12 
13:04:12 ... Panic: Fixture has panicked (see related PANIC)
13:04:12 
13:04:12 ----------------------------------------------------------------------

thaJeztah
thaJeztah approved these changes Aug 15, 2019
View reviewed changes
Copy link
Member
@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@thaJeztah
Copy link
Member

oh! looks like there's a linting issue

[2019-08-15T19:10:30.016Z] builder/dockerfile/copy.go:1::warning: file is not gofmted with -s (gofmt)
[2019-08-15T19:10:30.016Z] builder/dockerfile/copy.go:1::warning: file is not goimported (goimports)
script returned exit code 1
echo 'Ensuring c

Fixups for 39695 - edit comments, redundant lines
b146b23 
1. Modify comments added in 5858a99
Windows Volume GUID path format is: \\?\Volume{<GUID Value>}\<path>
Rewrote the example given in comments to conform to the format..

2. Remove two redundant asserts[assert.NilError]. They are redundant
because the last statement will not change the value of err.

Signed-off-by: Vikram bir Singh <vikrambir.singh@docker.com>
@vikramhh
Copy link
Author

gofmt issue fixed [was due to dangling space at end of line]. Squashed, rebased and force pushed to vikramhh:39695-fix

thaJeztah
thaJeztah approved these changes Aug 16, 2019
View reviewed changes
Copy link
Member
@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! still LGTM

@vikramhh vikramhh mentioned this pull request Aug 16, 2019
Closed
yongtang
yongtang approved these changes Aug 17, 2019
View reviewed changes
Copy link
Member
@yongtang yongtang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@yongtang yongtang merged commit 4760db0 into moby:master Aug 17, 2019
@thaJeztah thaJeztah added this to the 20.03.0 milestone Apr 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thaJeztah thaJeztah thaJeztah approved these changes

@yongtang yongtang yongtang approved these changes

@tonistiigi tonistiigi Awaiting requested review from tonistiigi tonistiigi is a code owner

Assignees
No one assigned
Labels
area/builder process/cherry-picked status/2-code-review
Projects
None yet
Milestone
20.10.0
Development

Successfully merging this pull request may close these issues.
5 participants
@vikramhh @ddebroy @thaJeztah @yongtang @GordonTheTurtle
0