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

Improve the flow of the containerd test with youki #1297

Merged
merged 4 commits into from
Nov 28, 2022

Conversation

utam0k
Copy link
Member

@utam0k utam0k commented Oct 24, 2022

No description provided.

@utam0k utam0k changed the title Makefile Improve the flow of the containerd test with youki Oct 24, 2022
@utam0k utam0k requested review from Furisto and YJDoc2 October 25, 2022 12:16
@utam0k
Copy link
Member Author

utam0k commented Oct 30, 2022

@YJDoc2 @Furisto May I ask you a favor to review this one?

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Oct 31, 2022

Hey @utam0k , I will review this by today or tomorrow 👍

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Nov 6, 2022

Hey @utam0k , Apologies for the delay 😓
I checked, and there are some issues / improvements that can be done

  • it seems that provision script is running each time we are doing the make. eg, after first run of make containerd-test , when we run it second time, it still hangs the same amount of time. When removed the | true part, it shows the log default: Running provisioner: bootstrap (shell)... also when running second time. we should really not run it twice. changing it as follows should make it work as expected :
config.vm.provision "shell" do |s|
      s.inline = <<-SHELL
...
  • can we have logs to be explicit in the test ? currently the logs are stored in a file, can we have them also printed to stdout, because otherwise it looks like the command has hanged without doing anything. For this removing the | true from first will also help.

Also a thing to note is I'm not sure how the syncing of youki dir with vm work, will need to check that it still works as expected after these changes.

Again, apologies for the delay, I unexpectedly got caught up in some work, and couldn't check this earlier.

Signed-off-by: utam0k <k0ma@utam0k.jp>
Signed-off-by: utam0k <k0ma@utam0k.jp>
@utam0k
Copy link
Member Author

utam0k commented Nov 8, 2022

Hey @utam0k , Apologies for the delay sweat I checked, and there are some issues / improvements that can be done

  • it seems that provision script is running each time we are doing the make. eg, after first run of make containerd-test , when we run it second time, it still hangs the same amount of time. When removed the | true part, it shows the log default: Running provisioner: bootstrap (shell)... also when running second time. we should really not run it twice. changing it as follows should make it work as expected :
config.vm.provision "shell" do |s|
      s.inline = <<-SHELL
...
  • can we have logs to be explicit in the test ? currently the logs are stored in a file, can we have them also printed to stdout, because otherwise it looks like the command has hanged without doing anything. For this removing the | true from first will also help.

Also a thing to note is I'm not sure how the syncing of youki dir with vm work, will need to check that it still works as expected after these changes.

Again, apologies for the delay, I unexpectedly got caught up in some work, and couldn't check this earlier.

Thanks for your check. You don't need to say apologize. I am just grateful that you are always willing to help.
I haven't found the time yet to respond, so I'll address it later.

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 YJDoc2 merged commit 502e4a2 into youki-dev:main Nov 28, 2022
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.

3 participants