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 easy way to test with K8s #1884

Merged
merged 2 commits into from
May 8, 2023
Merged

Add easy way to test with K8s #1884

merged 2 commits into from
May 8, 2023

Conversation

utam0k
Copy link
Member

@utam0k utam0k commented May 6, 2023

You can confirm this script:

  • k3s version
$ make test/k3s
[...]

$ make test/k3s/clean
  • kind version
$ make test/k8s/deploy
[...]

$ make test/k8s/clean

Relates: #730

Signed-off-by: utam0k <k0ma@utam0k.jp>
image: nginx:1.16.1
ports:
- containerPort: 80
automountServiceAccountToken: false
Copy link
Member Author

Choose a reason for hiding this comment

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

Deleting this line, unfortunately, youki gives an error 😭

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you have an idea why? We should track this as an issue.

@codecov-commenter
Copy link

codecov-commenter commented May 6, 2023

Codecov Report

Merging #1884 (29cf869) into main (5edbc9e) will decrease coverage by 0.01%.
The diff coverage is 12.50%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1884      +/-   ##
==========================================
- Coverage   67.70%   67.70%   -0.01%     
==========================================
  Files         124      124              
  Lines       14205    14206       +1     
==========================================
  Hits         9618     9618              
- Misses       4587     4588       +1     

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

@yihuaf yihuaf left a comment

Choose a reason for hiding this comment

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

Great! One thing to note is that this is more makefile. @YJDoc2 How is the progress on the justfile migration? Should we wait?

echo "CONTAINERD_NAMESPACE='default'" | sudo tee /etc/systemd/system/k3s-runwasi.service.env && \
echo "NO_PROXY=192.168.0.0/16" | sudo tee -a /etc/systemd/system/k3s-runwasi.service.env && \
sudo systemctl daemon-reload && \
sudo systemctl restart k3s-youki && \
Copy link
Collaborator

Choose a reason for hiding this comment

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

In general, I recommend separate the install step from the actual test step. We can create a prepare target that sets up the k3s environment for the test, then make test should only check the dependency and run the test. In this way, the test can run multiple times without repeating the install step.

Copy link
Member Author

Choose a reason for hiding this comment

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

@YJDoc2 Since I can't take any more time today, but I don't want to interrupt your work, can I ask you to fix it with just PR?

@@ -204,7 +204,7 @@ pub fn container_init_process(
// before pivot_root is called. This runs in the container namespaces.
if let Some(hooks) = hooks {
hooks::run_hooks(hooks.create_container().as_ref(), container)
.context("Failed to run create container hooks")?;
.context("failed to run create container hooks")?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't need this since these will be replaced with thiserror. Same for the others as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Prioritize the merge for subsequent work 🙇

image: nginx:1.16.1
ports:
- containerPort: 80
automountServiceAccountToken: false
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you have an idea why? We should track this as an issue.

@YJDoc2
Copy link
Collaborator

YJDoc2 commented May 8, 2023

@yihuaf The work is almost done, however I will take today/tomorrow to update the PR. In any case this should not be stopped for justfile. I'll update my PR for these changes as needed.

Once this is ready to go, please merge without waiting on Justfile PR 👍

@utam0k
Copy link
Member Author

utam0k commented May 8, 2023

#1890
#1884 (comment)

@utam0k utam0k merged commit 72a5fec into containers:main May 8, 2023
10 checks passed
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.

None yet

4 participants