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

[skip ci] semantics is badly broken #2944

Closed
timotheecour opened this issue Apr 24, 2020 · 16 comments
Closed

[skip ci] semantics is badly broken #2944

timotheecour opened this issue Apr 24, 2020 · 16 comments

Comments

@timotheecour
Copy link

timotheecour commented Apr 24, 2020

Please do not close this issue until this is either fixed or a valid workaround is offered and confirmed to work

The same issue has been reported several times (#2441 and #858) but the issue is still there.

According to https://developercommunity.visualstudio.com/content/problem/714477/pr-commit-with-skip-ci-in-message-still-triggers-a.html this is by design:

PRs currently do not honor [skip ci] keywords. This is to ensure that pipeline policies on PRs cannot be circumvented by including [skip ci] in the PR commits.

However that design is flawed because:

it goes against industry standard, is surprising, and doesn't play well with other CI

see #858 (comment)

Basically most of the others CI use [ci skip] or [skip ci] (e.g. Travis, CircleCI, Concourse)
Some dependencies use [ci skip] as a pattern to skip automatically versioning from their CI, why not make VSTS also accept it?

it's not what your users want

everyone is complaining about this:

eg:

This behavior is undesirable, regardless of whether it was by design or by oversight [...]. But even though that commit message specifies [skip ci] or NO_CI, CI build triggers are still fired. It makes no sense why this commit-via-PR shouldn't have to play by the same rules as other commits.

or this: #1270 (comment)

@kelliejos Why do you think that you know what end-users want? I want use skippers for all builds.
Thanks, but for GitHub PRs, it looks really weird when [CI skip] is ignored by Azure Pipelines but honoured by Travis CI and AppVeyor.

it invites dangerous workarounds

  • this causes workarounds like this:

The workaround for me is painful, and risky: 1). disable all policies on master branch (several dozen), 2). commit changes directory to master without benefit of a code review, which is why I want the PR!, and 3). re-enable policies on master branch.

it slows down development

because CI runs regardless what we specified

it's illogical

It is very strange that this feature skips CI build if any commit in the history has one of the indicator strings. This means, for example, that backporting a commit with [ci skip] onto a release branch skips CI for that branch even if the following 10 commits are un-skipped. Travis only skips if the most recent commit contains the skip indicator.

the rationale given makes no sense

This is to ensure that pipeline policies on PRs cannot be circumvented by including [skip ci] in the PR commits.

We don't need this babysitting; it's a PR, it still needs to be merged and it's trivial for a reviewer to check whether the CI was run or not (or whether last commit message message contained [ci skip]; reviewer can always request to change the last commit msg if he disagrees.

There's still a reviewer involved. Second guessing humans isn't helpful. Example: on a repo where I'm a maintainer I made an edit to a plan text file. I want to send it via a PR so it's visible to other maintainers that I'm making a change (pushing directly to master is considered bad form). But I don't need to trigger hours of CPU time worth of CI builds. I'm 99.99% sure I'm not breaking anything. And in the very unlikely case I did break something, I can just go back and fix it or revert the change.

it's not even documented

https://docs.microsoft.com/en-us/azure/devops/pipelines/repos/github?view=azure-devops&tabs=yaml#skipping-ci-for-individual-commits makes no mention of it not working for PR's

it will save you money and is good for the environment

...by avoiding wasting CI cycles. I mean, who doesn't want to save the environment ;-)


proposal

either fix the behavior of [ci skip] or provide a simple way to enable honoring [ci skip]for PR commit messages in azure pipelines, eg allow_pr_ci_skip: true

@cal5barton
Copy link

According to the docs https://docs.microsoft.com/en-us/azure/devops/pipelines/repos/azure-repos-git?view=azure-devops&tabs=yaml#skipping-ci-for-individual-commits this is already supposed to work, however, it does not.

@gaikovoi
Copy link

You can use next task condition to skip CI builds after PR is complited:
condition: not(and(in(variables['Build.Reason'], 'IndividualCI', 'BatchedCI'), contains(variables['Build.SourceVersionMessage'], 'NO_CI')))

It will help to skip unneeded tasks when magic 'NO_CI' present in the commit comment. It doesn't work on job or stage level as variable Build.SourceVersionMessage got populated only when the job started.

@msibrichins
Copy link

Any progress on this issue?

@anatolybolshakov
Copy link
Contributor

Hi everyone, we are working currently on more prioritized issues, but going to pick it up soon.

@timotheecour
Copy link
Author

@anatolybolshakov can you please define "soon"?

@gaikovoi are you sure #2944 (comment) works? Build.SourceVersionMessage for a PR looks like:

Merge d66b422899c095fffeeb6a1448a0af113e0eab7f into d58ad52ba31cc0d11be346d23f491e8b0793fefa

as observed by other people, eg #858 (comment)

@gaikovoi
Copy link

@timotheecour this is a condition to skip CI build triggered after completed PR. The condition will be evaluated as 'true' for PR build.

@timotheecour
Copy link
Author

i see. In the meantime I came up with this: nim-lang/Nim#17561, feedback welcome.

it's obviously not as good as a builtin [skip CI] for several reasons:

  • slower since it requires a clone (depth >= 2 needed, see PR)
  • more importantly, adds complexity/boilerplate in each pipeline

@MirKml
Copy link

MirKml commented Apr 6, 2021

It seems doesn't work even in own Azure Devops Git repositories, see screenshot. Merge commit message on HEAD with [skip ci] and all pipelines are triggered.
This was merge of docker files regarding some manual test, no ci trigger necessary but, all 12 pipelines triggered. 😟
image

@ihendriks1
Copy link

Not sure if this is the correct place to add this but:

When I merge a pull request (say, to master), and that PR contains a commit which has [skip ci], it doesn't trigger CI for that entire merge commit. The merge commit message does not contain the skip ci at all, so I don't understand why it behaves this way.

Anyone have any solution for this problem?

@StephenHodgson
Copy link

any update on this?

@ihendriks1
Copy link

ihendriks1 commented Aug 16, 2021

Not the most useful but someone from Microsoft has reproduced the problem and confirmed my issue (above) is a bug.

He created an issue for the devs here, if you want to follow it.

Besides that it's just broken atm. It works fine if you use an Azure Git repo, or GitHub actions. But that's usually not really a reasonable solution.

@rgommers
Copy link

We've been using this workaround in SciPy (cargo culted from some other scientific Python projects):

stages:

- stage: Check
  jobs:
    - job: Skip
      pool:
        vmImage: 'ubuntu-18.04'
      variables:
        DECODE_PERCENTS: 'false'
        RET: 'true'
      steps:
      - bash: |
          git_log=`git log --max-count=1 --skip=1 --pretty=format:"%B" | tr "\n" " "`
          echo "##vso[task.setvariable variable=log]$git_log"
      - bash: echo "##vso[task.setvariable variable=RET]false"
        condition: or(contains(variables.log, '[skip azp]'), contains(variables.log, '[azp skip]'), contains(variables.log, '[skip ci]'), contains(variables.log, '[ci skip]'))
      - bash: echo "##vso[task.setvariable variable=start_main;isOutput=true]$RET"
        name: result

- stage: Main
  condition: and(succeeded(), eq(dependencies.Check.outputs['Skip.result.start_main'], 'true'))
  dependsOn: Check

  ... <continue with the actual functionality> ...

Less tedious than adding condition: to every individual job or task.

@Yasston
Copy link

Yasston commented Oct 24, 2021

+1, it's so annoying to have PR-triggered builds run even when [skip ci] is in the commit message. Can't believe it's been more than a year and still no fix... Truly sad...

@drtbz
Copy link

drtbz commented Feb 2, 2022

Any chance of an update on this sometime soon? Not being able to skip PR triggered pipelines breaks planned workflows for us. Like when a pipeline to automatically update documentation commits to a PR and then causes another run of itself..

I'd argue that over a year's wait is not soon

@anatolybolshakov
Copy link
Contributor

Hi everyone, just re-checked requirements here - the pipeline agent does not handle ci checks run, so it sounds like an enhancement for Azure DevOps itself. For such requests https://developercommunity.visualstudio.com/search?space=21 would be a better place to open a ticket - since this repo is mostly for the agent issues/feature requests.

@anatolybolshakov
Copy link
Contributor

Let me close it here as external issue - please feel free to ask any further questions about the agent here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests