CI: some cirrus setup improvements/speedups #23805
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
openshift-ci
bot
added
do-not-merge/work-in-progress
Luap99
force-pushed
the
cirrus-timeouts
branch
from
02104fc to
840c89e
Compare
|
renovate config check seems to be working (added a bad commit in there that contained invalid json): https://cirrus-ci.com/task/5678090586685440 |
Luap99
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
@edsantiago PTAL |
edsantiago
approved these changes
Aug 29, 2024
contrib/cirrus/runner.sh
Outdated
| @@ -29,11 +29,26 @@ function _run_validate-source() { | |||
|
|
|||
| # make sure PRs have jira links (if needed for branch) | |||
| showrun make test-jira-links-included | |||
|
|
|||
| # Defined by Cirrus-CI for all tasks | |||
There was a problem hiding this comment.
All the commits look great, except this one. I realize validate is only run in PRs (via .cirrus.yml) but I still don't like this here. Could I ask you to add, perhaps on line 25, req_env_vars CIRRUS_CHANGE_IN_REPO PR_BASE_SHA and a nice comment like "This target should only run on PRs" ? Not a blocker, just a request from future me.
There was a problem hiding this comment.
yes that makes sense, it certainly should create a proper error message if it is ever called from the wrong context instead of failing in a undefined way.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: edsantiago, Luap99 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Two weeks ago we had a weird hang in the macos build job on the persitent worker. The task just hang for an hour wasting time. Most tests are fast so we do not need/want such high timeouts. Therefore drop the default timeout to 20 minutes. The integration task also should take under 20m so we can remove the longer timeout there as well. Some system tests need a bit over 30m currently, timeout is set to 35m. For machine tests we use 30m on linux, 45m on windows and 35m on macos to have some extra room there as machine tests have a much higher run to run variance. Signed-off-by: Paul Holzinger <[email protected]>
It is not used by anybody so we do not have to store these and can safe some time by not having to generate it even if it is just ~500ms. Signed-off-by: Paul Holzinger <[email protected]>
They do not add much value to users, first of it compiles podman with cgo disabled which means the included the podman binary is unusable either way. The only goal of the build job is to ensure we can compile on all arches, i.e. go build tags adn types work correctly. The upload if these artifacts alone take over 90s so let's get rid of them to speed up the total CI time. Signed-off-by: Paul Holzinger <[email protected]>
We do build and test aarch64 and x86_64 natively so the cross job doesn't seem to add value. Signed-off-by: Paul Holzinger <[email protected]>
This doesn't help us at all, first the list is outdated. AFAICT we no longer connect to docker.io, registry.fedoraproject.org or podman.cachix.org (seems to be a cache site for nix.dev?) anywhere in our tests. Second a simple port check is not helpful, in the most cases the CDN's and or load balancer accept connections but return internal server errors when the registy goes down. This is very similar to commit 5b6de98. Signed-off-by: Paul Holzinger <[email protected]>
The renovate config is used for the renovate bot, validating this in the prior fedora prebuild setp is just confusing and hidden. The problem is this image is very big so it is slow to download/extract. To speed things up given it is only a single file we check the diff if we even changed it. Now one could argue this should be part of the validate Makefile target but I given the size I do not want this run by default and I am not sure if we should do the diff check in the Makefile. Lastly remove -it, these is meant for interactive use and throws a warning here because we have no actual tty attached. Signed-off-by: Paul Holzinger <[email protected]>
Since commit 55ad0d6 we do the conditions in the cirrus.yml directly so there is no longer any need for this. Signed-off-by: Paul Holzinger <[email protected]>
Luap99
force-pushed
the
cirrus-timeouts
branch
from
840c89e to
6a0ab6f
Compare
|
/lgtm |
stale-locking-app
bot
added
the
locked - please file new issue/PR
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
see commits
Does this PR introduce a user-facing change?