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

gha: Run integration tests in GHA #22900

Merged
merged 1 commit into from Feb 2, 2023
Merged

gha: Run integration tests in GHA #22900

merged 1 commit into from Feb 2, 2023

Conversation

sayboras
Copy link
Member

@sayboras sayboras commented Jan 1, 2023 •
edited

Description

Current travis CI is used to run integration-tests target only, however, this can be achieved with GitHub action easily (at least for amd64).

Signed-off-by: Tam Mach tam.mach@cilium.io

Testing

Both jobs are taking around 20 mins to complete, GHA job is having a shorter bootstrap time compared to Travis, hence from UX point of view, GHA provides a faster feedback, not to mention that Cilium contributors are already familiar with GHA ops such as re-run, checking logs, etc.

Note

Not sure if arm64 run in travis provides any added value, if not, we can just remove travis completely and refactor the scripts a little bit.

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jan 1, 2023
@sayboras sayboras force-pushed the tam/replace-travis branch 4 times, most recently from fba6c07 to 068a4cd Compare January 1, 2023 05:05
@sayboras sayboras changed the title gha: Replace travis CI gha: Run integration tests in GHA Jan 1, 2023
@sayboras sayboras force-pushed the tam/replace-travis branch 2 times, most recently from 51506ee to 607dee2 Compare January 1, 2023 05:42
@sayboras sayboras added area/CI Continuous Integration testing issue or flake release-note/misc This PR makes changes that have no direct user impact. labels Jan 1, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jan 1, 2023
@sayboras sayboras force-pushed the tam/replace-travis branch 5 times, most recently from 922fe06 to 38e17d8 Compare January 1, 2023 06:14
@sayboras sayboras marked this pull request as ready for review January 1, 2023 06:18
@sayboras sayboras requested review from a team as code owners January 1, 2023 06:18
@sayboras
Copy link
Member Author

sayboras commented Jan 1, 2023

Full CI is not required, changes are related to travis and GHA only.

nebril
nebril approved these changes Jan 9, 2023
View reviewed changes
Copy link
Member

@nebril nebril left a comment

Choose a reason for hiding this comment

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

I guess we don't care about having too many GHA running anymore?

@sayboras
Copy link
Member Author

I guess we don't care about having too many GHA running anymore?

I think so, it's a perk of being CNCF project if I am not wrong.

joestringer
joestringer reviewed Jan 20, 2023
View reviewed changes
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

I'd have to take another look at the GHA when I'm fresher, but I noticed a couple of things while glancing through.

I wonder about the output, is it reasonable to have 16K lines of output in the workflow run? I wonder if we could sprinkle some make --quiet and/or V=0 make magic to quieten things down. On Travis we seem to get around 800 lines, so something must be subtly different about the environment variables or something, such that we don't flood the logs wit hso much output. In this case, the GitHub interface visibly lags when you go to inspect the results.

This failed at the end of the test too, should we install goveralls?

./.travis/build.sh: line 10: /home/runner/gopath/bin/goveralls: No such file or directory

.github/workflows/integration-test.yaml Outdated Show resolved Hide resolved
.github/workflows/integration-test.yaml Outdated Show resolved Hide resolved
@sayboras
Copy link
Member Author

This failed at the end of the test too, should we install goveralls?

I think this step failed in travis as well, so I just remove it in script.

error parsing coverage: open coverage-all.out: no such file or directory

@sayboras
Copy link
Member Author

A lot of failures in GHA are due to SBOM failure in image jobs, the new IntegrationTest is successfully ran in https://github.com/cilium/cilium/actions/runs/3966354144/jobs/6797060899

@pchaigno
Copy link
Member

pchaigno commented Feb 1, 2023

Given

I wonder about the output, is it reasonable to have 16K lines of output in the workflow run? I wonder if we could sprinkle some make --quiet and/or V=0 make magic to quieten things down.

Was this addressed? If yes, I guess this is ready to merge, no?

@sayboras
Copy link
Member Author

sayboras commented Feb 2, 2023

Was this addressed? If yes, I guess this is ready to merge, no?

Yes, this point is addressed, the recent run contains only 500+ log lines.

@sayboras
gha: Run integration tests in GHA
8c92c73 
Current travis CI is used to run integration-tests target only, however,
this can be achieved with github action easily (at least for amd64).

Signed-off-by: Tam Mach <tam.mach@cilium.io>
@sayboras
Copy link
Member Author

sayboras commented Feb 2, 2023

The changes here are only to add new IntegrationTest GHA, full CI is not required.

As recent run is good and all required reviews are in, I am marking this ready to merge.

https://github.com/cilium/cilium/actions/runs/4071443474/jobs/7013252483

@sayboras sayboras added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/ci This PR makes changes to the CI. and removed release-note/misc This PR makes changes that have no direct user impact. labels Feb 2, 2023
michi-covalent
michi-covalent approved these changes Feb 2, 2023
View reviewed changes
Copy link
Contributor

@michi-covalent michi-covalent left a comment

Choose a reason for hiding this comment

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

❤️❤️❤️

@sayboras sayboras mentioned this pull request Feb 2, 2023
Closed
@pchaigno pchaigno merged commit 2ed5f96 into cilium:master Feb 2, 2023
@sayboras sayboras deleted the tam/replace-travis branch February 2, 2023 09:53
@aanm aanm added the needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch label Mar 12, 2024
@gandro
Copy link
Member

gandro commented Mar 19, 2024 •
edited

@aanm This already seems to have been applied to 1.13 via #31309?

In any case, I'll mark as backport author

@gandro gandro added the backport/author The backport will be carried out by the author of the PR. label Mar 19, 2024
@gandro gandro mentioned this pull request Mar 19, 2024
Merged
8 tasks
@aanm
Copy link
Member

aanm commented Mar 19, 2024

@gandro oh yes they were! Sorry.

@aanm aanm added backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. and removed backport/author The backport will be carried out by the author of the PR. needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch labels Mar 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joestringer joestringer joestringer approved these changes

@nebril nebril nebril approved these changes

@michi-covalent michi-covalent michi-covalent approved these changes

Assignees
No one assigned
Labels
area/CI Continuous Integration testing issue or flake backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/ci This PR makes changes to the CI.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@sayboras @pchaigno @gandro @aanm @joestringer @nebril @michi-covalent