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

Added recursive mount attr test #1428

Merged
merged 6 commits into from
Jan 30, 2023
Merged

Added recursive mount attr test #1428

merged 6 commits into from
Jan 30, 2023

Conversation

higuruchi
Copy link
Contributor

@higuruchi higuruchi commented Dec 19, 2022 •
edited
Loading

Added test of rro mount option that implemented in #1398.
/tmp/ is used as the bind mount path used for testing.

Now, this PR implement rro option test only.
And if this test is correct, add other option test(rrw, rnosuid...).

  • rro
  • rrw
  • rnosuid
  • rsuid
  • rnodev
  • rdev
  • rnoexec
  • rexec
  • rnodiratime
  • rdiratime
  • rrelatime
  • rnorelatime
  • rnoatime
  • ratime
  • rstrictatime
  • rnostrictatime
  • rnosymfollow
  • rsymfollow

Signed-off-by: higuruchi [email protected]

@higuruchi higuruchi marked this pull request as draft December 19, 2022 08:52
@higuruchi higuruchi changed the title Added recursive mount attr test. Added recursive mount attr test Dec 19, 2022
@higuruchi higuruchi force-pushed the Add/Recursive_mount_attrs_test branch from ed04ac3 to f897dc2 Compare December 19, 2022 12:00
@higuruchi higuruchi force-pushed the Add/Recursive_mount_attrs_test branch from f897dc2 to 88452c5 Compare December 19, 2022 12:19
@utam0k utam0k requested a review from YJDoc2 December 19, 2022 12:30
@higuruchi higuruchi marked this pull request as ready for review December 19, 2022 12:34
YJDoc2
YJDoc2 reviewed Dec 19, 2022
View reviewed changes
Copy link
Collaborator

@YJDoc2 YJDoc2 left a comment

Choose a reason for hiding this comment

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

Hey, overall seems fine, there are some changes suggested and a couple of questions. Please take a look, and add tests for remaining mount types as well.

tests/rust-integration-tests/runtimetest/src/tests.rs Outdated Show resolved Hide resolved
tests/rust-integration-tests/runtimetest/src/tests.rs Outdated Show resolved Hide resolved
tests/rust-integration-tests/runtimetest/src/tests.rs Outdated Show resolved Hide resolved
tests/rust-integration-tests/runtimetest/src/tests.rs Outdated Show resolved Hide resolved
tests/rust-integration-tests/runtimetest/src/tests.rs Outdated
eprintln!("mounts_recursive rro error: {}", e);
}
}
"rrw" => { /* TODO... */ }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Otherwise test looks fine, so please add other tests in this PR as well 👍

tests/rust-integration-tests/runtimetest/src/main.rs Outdated Show resolved Hide resolved
tests/rust-integration-tests/integration_test/src/tests/hostname/mod.rs Show resolved Hide resolved
@higuruchi higuruchi force-pushed the Add/Recursive_mount_attrs_test branch from 59a7afa to 0356169 Compare December 20, 2022 05:11
@codecov-commenter
Copy link

codecov-commenter commented Dec 20, 2022 •
edited
Loading

Codecov Report

Merging #1428 (ccbd26b) into main (764cfe8) will decrease coverage by 0.28%.
The diff coverage is n/a.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1428      +/-   ##
==========================================
- Coverage   69.00%   68.73%   -0.28%     
==========================================
  Files         119      120       +1     
  Lines       13041    13094      +53     
==========================================
+ Hits         8999     9000       +1     
- Misses       4042     4094      +52

@higuruchi higuruchi force-pushed the Add/Recursive_mount_attrs_test branch from 0356169 to f645736 Compare December 21, 2022 15:46
@higuruchi higuruchi marked this pull request as draft December 21, 2022 15:48
@higuruchi higuruchi force-pushed the Add/Recursive_mount_attrs_test branch from f645736 to 882032c Compare December 22, 2022 07:06
@higuruchi higuruchi force-pushed the Add/Recursive_mount_attrs_test branch 2 times, most recently from 2f0adda to fe498b6 Compare December 23, 2022 05:21
@higuruchi higuruchi force-pushed the Add/Recursive_mount_attrs_test branch from fe498b6 to 40e113a Compare December 23, 2022 11:41
@YJDoc2
Copy link
Collaborator

YJDoc2 commented Jan 24, 2023

Hey @higuruchi sorry I haven't looked at this in a while. Is there any issue you are facing? If you are busy and cannot work on this, it is ok too. We can add the partial tests and add another task for remaining.

If there is any issue, feel free to ping or mention!

@higuruchi
Copy link
Contributor Author

@YJDoc2
Also, I am unlikely to be able to work on it for a few weeks.
After fixing the clap problem, can I issue the rest?

@YJDoc2 YJDoc2 marked this pull request as ready for review January 24, 2023 07:29
@YJDoc2
Copy link
Collaborator

YJDoc2 commented Jan 24, 2023

Hey @higuruchi No worries. If you are fine with it , we can merge this now, and when you get time you can come back and work on the rest if you want. In the meantime, if anyone else picks up the missing cases, we can progress that way too.
Thanks for setting up the basic test and doing all this work ❤️

@higuruchi
Fix error messages
ccbd26b 
Signed-off-by: higuruchi <[email protected]>
@higuruchi higuruchi requested a review from YJDoc2 January 24, 2023 07:58
@higuruchi
Copy link
Contributor Author

higuruchi commented Jan 27, 2023 •
edited
Loading

@YJDoc2

If you are fine with it , we can merge this now

Please merge if there is no problem.
And i will add more tests as time permits.

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Jan 27, 2023

Hey @higuruchi , sure, the PR seems mostly fine , its just that I didn't have time to take a final look. I'll try to merge this today.

Please merge if there is no problem.

YJDoc2
YJDoc2 approved these changes Jan 30, 2023
View reviewed changes
Copy link
Collaborator

@YJDoc2 YJDoc2 left a comment

Choose a reason for hiding this comment

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

lgtm 👍

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Jan 30, 2023

Hey @higuruchi , I'm merging this. Thank you for the contribution !

@YJDoc2 YJDoc2 merged commit 3675a51 into containers:main Jan 30, 2023
@higuruchi
Copy link
Contributor Author

@YJDoc2 May I raise an issue showing the rest of the tests?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@YJDoc2 YJDoc2 YJDoc2 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@higuruchi @codecov-commenter @YJDoc2