-
Notifications
You must be signed in to change notification settings - Fork 43
cirrus: some fixes #28
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
Conversation
First a change in .cirrus.yml must run all tests we cannot skip them as we might chnage what tasks are being executed and how, i.e. an CI image update must always run all tests. Second use only_if and not skip, skip will bloat the task overview with plenty of tasks so it is harder to see what was actully run then. that is consistent with how we do it in podman. And drop the vendor task as we do not do vendor anymore. Signed-off-by: Paul Holzinger <pholzing@redhat.com>
Also drop the vendor step, we don't need that anymore. Signed-off-by: Paul Holzinger <pholzing@redhat.com>
And update the CIRRUS_WORKING_DIR to use the proper repo path. Signed-off-by: Paul Holzinger <pholzing@redhat.com>
To speed up tests. Signed-off-by: Paul Holzinger <pholzing@redhat.com>
mtrmac
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks plausible otherwise — I didn’t review the test logs, though.
| skip: "!changesInclude('storage/**', 'image/**')" | ||
| # The git-validation tool doesn't work well on branch or tag push, | ||
| # under Cirrus-CI, due to challenges obtaining the starting commit ID. | ||
| # Only do validation for PRs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was dropping this extra condition intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now yes, I was to lazy to make custom conditions here because I though this job and the storage/common validation should all be merged together. Because well validating the commits here isn't a image specific thing so my next step is to consolidate the validate/lint jobs into one thus for now I think it is fine to leave it like that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*shrug* fine. I’m all for consolidating / improving the CI scripting, and it’s not very clear what this is warning about. I guess we’ll find out :) The way I read it, we should still be able to merge PRs on stable branches, so we won’t be scrambling to do a backport.
lsm5
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
/LGTM |
First a change in .cirrus.yml must run all tests we cannot skip them as we might chnage what tasks are being executed and how, i.e. an CI image update must always run all tests.
Second use only_if and not skip, skip will bloat the task overview with plenty of tasks so it is harder to see what was actully run then. that is consitent with how we do it in podman.