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

Add domainname test #1544

Merged
merged 9 commits into from
Jan 21, 2024
Merged

Add domainname test #1544

merged 9 commits into from
Jan 21, 2024

Conversation

higuruchi
Copy link
Contributor

Add domainname test.
nix does't have getdomainname function, So used libc crate.

Signed-off-by: higuruchi [email protected]

@higuruchi higuruchi marked this pull request as draft February 2, 2023 14:12
@higuruchi higuruchi force-pushed the Add/domainname_test branch 3 times, most recently from b7dc747 to 09a3be6 Compare February 2, 2023 15:22
@utam0k
Copy link
Member

utam0k commented Feb 2, 2023

@higuruchi Thanks for your cool PR, but we have to wait for runc side. opencontainers/runc#3600

Signed-off-by: higuruchi <[email protected]>
@yihuaf
Copy link
Collaborator

yihuaf commented May 3, 2023

@higuruchi Thanks for your cool PR, but we have to wait for runc side. opencontainers/runc#3600

@utam0k I see the runc PR is now merged. Should we get this PR merged as well? @higuruchi do you have time to follow up? If not, I can help to clean this PR up and get it merged.

@utam0k
Copy link
Member

utam0k commented May 3, 2023

@yihuaf We neet to wait for the release from runc's side. Thanks for your catch-up 🙏

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Jul 24, 2023

@utam0k I think we can proceed with now? Or is runc release with this is still not done?

@utam0k
Copy link
Member

utam0k commented Jul 27, 2023

@utam0k I think we can proceed with now? Or is runc release with this is still not done?

I think we can process this PR!

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Jul 27, 2023

@higuruchi let me know if you can continue working on this for re-basing and fixing conflicts. In case you are busy with something else or cannot work on this, I can take it up so let me know 👍

@higuruchi
Copy link
Contributor Author

I can continue this work. Thank you for letting me know.

Comment on lines 34 to 37
_ => eprintln!(
"error due to unexpected execute test name: {}",
execute_test
),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this got duplicated in the master merge, and clippy is giving error...

@codecov-commenter
Copy link

codecov-commenter commented Jul 28, 2023

Codecov Report

Merging #1544 (824e1fb) into main (2723aa4) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1544   +/-   ##
=======================================
  Coverage   65.50%   65.50%           
=======================================
  Files         133      133           
  Lines       16916    16916           
=======================================
  Hits        11081    11081           
  Misses       5835     5835           

Signed-off-by: higuruchi <[email protected]>
@higuruchi higuruchi force-pushed the Add/domainname_test branch 2 times, most recently from 59c70b3 to 5aae0a5 Compare July 28, 2023 07:35
@higuruchi higuruchi force-pushed the Add/domainname_test branch 2 times, most recently from e51fba1 to 8d8e545 Compare July 28, 2023 10:16
@higuruchi
Copy link
Contributor Author

@YJDoc2

integration_tests_validation fails because the latest version of runc, v1.1.8, seems does not include domain name pull request. If i run the tests on the main branch binary, they will all pass. Wait for the domain name pull request to be added to the new release?

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Jul 29, 2023

I think they are waiting for a minor release to roll the domainname feature out, and recent releases have been patch , so they didn't include it. I guess we will simply have to wait. @utam0k wdyt? Should we instead move to the main branch with a fixed sha or only use releases? I feel releases would be better because that is what actually being shipped.

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Jan 16, 2024

@higuruchi May I ask you to rebase it to current master and do the following to fix the failing runc test :
keep everything same, but comment out the addition of the test in main.rs . I'll make it work for both in a separate PR later.
If you are busy with something else, I can take this up and do the necessary changes, let me know.
Thanks a lot 🙏

@higuruchi
Copy link
Contributor Author

@higuruchi May I ask you to rebase it to current master and do the following to fix the failing runc test : keep everything same, but comment out the addition of the test in main.rs . I'll make it work for both in a separate PR later. If you are busy with something else, I can take this up and do the necessary changes, let me know. Thanks a lot 🙏

@YJDoc2
I can't make any changes to youki at the moment, so would you mind if I ask you to make a correction?

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Jan 16, 2024

I can't make any changes to youki at the moment, so would you mind if I ask you to make a correction?

No worries, I'll take care of this. Thanks a lot for the contributions 🙏

@higuruchi
Copy link
Contributor Author

I can't make any changes to youki at the moment, so would you mind if I ask you to make a correction?

No worries, I'll take care of this. Thanks a lot for the contributions 🙏

Thanks a lot 🙏

@YJDoc2 YJDoc2 marked this pull request as ready for review January 17, 2024 05:37
Signed-off-by: Yashodhan Joshi <[email protected]>
Signed-off-by: Yashodhan Joshi <[email protected]>
@YJDoc2 YJDoc2 requested a review from a team January 17, 2024 08:23
Copy link
Member

@utam0k utam0k left a comment

Choose a reason for hiding this comment

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

LGTM. Please fix the conflict.

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Jan 21, 2024

Thanks a lot @utam0k for review!
Thanks @higuruchi for adding this, and apologies, it took us so long to get this merged 🙏

@YJDoc2 YJDoc2 merged commit c2e8670 into youki-dev:main Jan 21, 2024
28 checks passed
@github-actions github-actions bot mentioned this pull request Jan 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants