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:DOCS] Cirrus: Ability to skip most tests for docs updates #7998

Merged
merged 1 commit into from
Oct 13, 2020

Conversation

cevich
Copy link
Member

@cevich cevich commented Oct 12, 2020

Warning: skip has non-obvious side-effects vs only_if:
https://cirrus-ci.org/guide/writing-tasks/#conditional-task-execution

The skip instruction can give a false sense of security by always
marking tasks as passed in the UI, even if they didn't run. In
contrast, the only_if condition will avoid creating the task
all -together; therefore, a problematic task's absense is more likely to
be noticed if it introduced a problem.

Note: I left the OSX build in because it actually does some platform-specific docs-manipulation we want exercised.

@mheon
Copy link
Member

mheon commented Oct 12, 2020

LGTM

@TomSweeneyRedHat
Copy link
Member

Is this taking over for #7797? Looks like it's working better.

@edsantiago
Copy link
Member

Looks like we collided. I went with skip because it was easy for my map maker to parse and because positive logic (skip if PR =~ CI:DOCS) is more readable and maintainable than negative (only if PR !~ CI:DOCS).

.cirrus.yml Outdated Show resolved Hide resolved
@edsantiago
Copy link
Member

LGTM, modulo one request for DRY. I like how this approach doesn't have all the nasty gray bullet clutter that skip does. Map attached for those who find it useful for review.
cirrus-map-pr7998

@cevich
Copy link
Member Author

cevich commented Oct 13, 2020

I went with skip because it was easy for my map maker to parse and because positive logic

@edsantiago warning...danger..danger: skip has non-obvious side-effects vs only_if:
https://cirrus-ci.org/guide/writing-tasks/#conditional-task-execution

The skip instruction can give a false sense of security by always marking tasks as passed in the UI, even if they didn't run. Therefore, this includes if the PR actually breaks something in one of those green-but-skipped tasks.

Consider the position of some future CI-failure analyst: They must go trolling back through multiple CI builds, trying to find the one that broke something. If the culprit was a [CI:DOCS] task, they are much more likely to notice if it did not run the broken task, as opposed to seeing all tasks are all green. That's why I deliberately chose to use only_if over skip, the former doesn't create the task at all, the later does and then lies about it's status.

***Warning***: `skip` has non-obvious side-effects vs `only_if`:
https://cirrus-ci.org/guide/writing-tasks/#conditional-task-execution

The skip instruction can give a false sense of security by always
marking tasks as passed in the UI, even if they didn't run. In
contrast, the `only_if` condition will avoid creating the task
all -together; therefore, a problematic task's absense is more likely to
be noticed if it introduced a problem.

Signed-off-by: Chris Evich <[email protected]>
@cevich
Copy link
Member Author

cevich commented Oct 13, 2020

Rebased and updated w/ Ed's anchor/alias suggestion. I also updated the commit and PR message to make clear the reasoning behind only_if vs skip usage.

@cevich
Copy link
Member Author

cevich commented Oct 13, 2020

Clarification: What I mean by false-success status can be seen in this PRs twin, in the WebUI: https://cirrus-ci.com/build/6731773184835584

@edsantiago
Copy link
Member

/lgtm
/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 13, 2020
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 13, 2020
@cevich
Copy link
Member Author

cevich commented Oct 13, 2020

@edsantiago is there some reason why we're holding on this? Did I forget something important again?

@edsantiago
Copy link
Member

No reason to hold, I just use that convention to allow you to hold-cancel or to solicit further review; i.e. to leave that decision up to you.

@cevich
Copy link
Member Author

cevich commented Oct 13, 2020

Oh whew! okay, thanks.

@cevich
Copy link
Member Author

cevich commented Oct 13, 2020

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 13, 2020
@edsantiago
Copy link
Member

/approve

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cevich, edsantiago

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 13, 2020
@openshift-merge-robot openshift-merge-robot merged commit 8fef35a into containers:master Oct 13, 2020
@cevich cevich deleted the add_ci_docs branch June 30, 2021 18:00
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 23, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants