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.

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

test(systest): avoid host volume mount in minio test #8569

Merged
merged 1 commit into from
Jan 5, 2023
Merged

test(systest): avoid host volume mount in minio test #8569

merged 1 commit into from
Jan 5, 2023

Conversation

mangalaman93
Copy link
Contributor

@mangalaman93 mangalaman93 commented Jan 4, 2023 •
edited
Loading

instead of a host mount, we do everything inside the container now. This avoids creating any files on the host and the delete error on during cleanup on the CI do not happen.

@github-actions github-actions bot added the area/testing Testing related issues label Jan 4, 2023
all-seeing-code
all-seeing-code approved these changes Jan 4, 2023
View reviewed changes
Copy link
Contributor

@all-seeing-code all-seeing-code left a comment

Choose a reason for hiding this comment

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

Looks good to me. Please take a look at some comments above.

skrdgraph
skrdgraph approved these changes Jan 4, 2023
View reviewed changes
Copy link
Contributor

@skrdgraph skrdgraph left a comment

Choose a reason for hiding this comment

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

@aman-bansal - this looks good to merge. Thanks!

The above comments are nitpicks .. and can be addressed when we move to compose tool later.

@skrdgraph
Copy link
Contributor

I have re-triggered a CI run .. once you have all passing - pls merge this off.

@coveralls
Copy link

coveralls commented Jan 4, 2023 •
edited
Loading

Coverage Status

Coverage: 64.308% (-0.1%) from 64.409% when pulling 26d1f15 on aman/minio into 32de2e4 on main.

@all-seeing-code
Copy link
Contributor

Test failure seems relevant to the change. @mangalaman93 highlighting it for you.

Caught output:
=== CONT  TestBulkCases/bulk_test_cases
    bulk_live_fixture.go:84: 
        	Error Trace:	bulk_live_fixture.go:84
        	            				bulk_live_cases.go:151
        	            				bulk_live_cases.go:52
        	Error:      	Received unexpected error:
        	            	Get "http://localhost:9001/data/?location=": dial tcp 127.0.0.1:9001: connect: connection refused
        	Test:       	TestBulkCases/bulk_test_cases
--- FAIL: TestBulkCases (7.87s)
    --- FAIL: TestBulkCases/bulk_test_cases (7.87s)
        --- PASS: TestBulkCases/bulk_test_cases/Pan_and_Jackson (0.10s)
        --- PASS: TestBulkCases/bulk_test_cases/Pan_only (0.00s)
        --- PASS: TestBulkCases/bulk_test_cases/Jackson_only (0.00s)
       ```

@skrdgraph
Copy link
Contributor

skrdgraph commented Jan 5, 2023 •
edited
Loading

I think this should be hitting minio:9001 instead of localhost:9001

Received unexpected error:
[1069](https://github.com/dgraph-io/dgraph/actions/runs/3840357871/jobs/6543986295#step:11:1070)
        	            	Get "http://localhost:9001/data/?location=": dial tcp 127.0.0.1:9001: connect: connection refused

@skrdgraph
Copy link
Contributor

related PR #8576 (comment)

@mangalaman93
test(live): avoid host volume mount in minio test
26d1f15 
The host mount would have files created as root causing the
cleanup to fail on the CI.
@mangalaman93
Copy link
Contributor Author

I have no idea how these tests have been passing. It could be because of differences in the docker version or something. I have fixed the tests now.

@skrdgraph
Copy link
Contributor

I have no idea how these tests have been passing. It could be because of differences in the docker version or something. I have fixed the tests now.

there was a bunch of docker related issues which we have also fixed in the t.go file. Mostly because the old team was using cmd exec from go-code ... and those cli parsers have changed overtime with newer docker builds. I made those changes with @dshekhar95 initially while setting up CI.

It could be something related to host-networking as well... sorry to have rabbit-holed you into this.

@mangalaman93
Copy link
Contributor Author

yeah, it is a bit cleaner now and I will spend some time after slash main alignment on improving our test infra.

@mangalaman93 mangalaman93 merged commit 900a8b3 into main Jan 5, 2023
@mangalaman93 mangalaman93 deleted the aman/minio branch January 5, 2023 08:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skrdgraph skrdgraph skrdgraph approved these changes

@all-seeing-code all-seeing-code all-seeing-code approved these changes

@akon-dey akon-dey Awaiting requested review from akon-dey

@darkn3rd darkn3rd Awaiting requested review from darkn3rd

@meghalims meghalims Awaiting requested review from meghalims

@matthewmcneely matthewmcneely Awaiting requested review from matthewmcneely

@billprovince billprovince Awaiting requested review from billprovince

Assignees
No one assigned
Labels
area/testing Testing related issues
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@mangalaman93 @skrdgraph @coveralls @all-seeing-code