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

ci: Test against various kernels on PR #373

Merged
merged 1 commit into from
Dec 19, 2023

Conversation

jschwinger233
Copy link
Member

Closes: #368

@jschwinger233
Copy link
Member Author

jschwinger233 commented Dec 19, 2023

~~ New workflow requires approval for running. ~~

I ran them on my personal fork jschwinger233#1, it looks code logic is fine but apt install broke from time to time on 5.15/5.10

@jschwinger233
Copy link
Member Author

@jschwinger233 jschwinger233 marked this pull request as ready for review December 19, 2023 13:20
@jschwinger233 jschwinger233 requested a review from a team as a code owner December 19, 2023 13:20
@sumire88
Copy link
Contributor

Hey @jschwinger233, thanks for the contribution. As far as I understood, are these proposed changes mainly for testing purpose?

dae-prow[bot]
dae-prow bot previously approved these changes Dec 19, 2023
Copy link
Contributor

@dae-prow dae-prow bot left a comment

Choose a reason for hiding this comment

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

🧪 Since the PR has been fully tested, please consider merging it.

@jschwinger233
Copy link
Member Author

are these proposed changes mainly for testing purpose?

That's right. Although not all cases are covered (only TCP + dip filter + socks5 are tested), this workflow gives a good sense of whether dae works under different kernels. If bpf verifier complains due to complexity issues, this workflow definitely will catch them. We can also extend test suite as a follow-up if we want.

push:
branches: [ main ]
pull_request:
branches: [ main ]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
branches: [ main ]
paths:
- "**/*.go"
- "**/*.c"
- "**/*.h"
- "go.mod"
- "go.sum"
- ".github/workflows/test.yml"

Add trigger conditions (based on files).

Comment on lines 4 to 5
push:
branches: [ main ]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
push:
branches: [ main ]

For trouble shooting purpose, we may do the kernel-related testing in the PR level to ensure we fix the incompatible issue before we merge it to main branch.

Copy link
Contributor

@miooochi miooochi left a comment

Choose a reason for hiding this comment

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

@jschwinger233 Thanks for the fabulous work. One little comment/suggestion from my end, would you like to rename test.yml to kernel-test.yml to clarify the purpose of the test?

Copy link
Contributor

@miooochi miooochi left a comment

Choose a reason for hiding this comment

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

LGTM

@miooochi
Copy link
Contributor

Once again appreciate all the efforts.

@miooochi miooochi merged commit a794c4c into daeuniverse:main Dec 19, 2023
5 checks passed
@jschwinger233 jschwinger233 deleted the gray/ci branch December 21, 2023 11:14
jschwinger233 added a commit to jschwinger233/dae that referenced this pull request Dec 26, 2023
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.

[Enhancement] Add CI test
3 participants