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

Update ci skip documentation in infrastructure.md #2297

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

wshanks
Copy link

@wshanks wshanks commented Sep 14, 2024

PR Checklist:

  • note any issues closed by this PR with closing keywords
  • if you are adding a new page under docs/ or community/, you have added it to the sidebar in the corresponding _sidebar.json file
  • put any other relevant information below

I noticed that when you use the bot to do non-packaging changes like update maintainer it only uses [ci skip]. So is it safe for the documentation to suggest only [ci skip] now?

Also, are the linked issues still relevant? #629 does not seem useful to me. It talks about some nuances with Travis, Circle, and Azure, but I am not sure what to take away as actionable. All my feedstocks only use Azure now. I think conda-forge/staged-recipes#1148 is similarly linked for nuances between CI systems.

Also, I think #498 can be closed since skipping CI is already in the docs.

@wshanks wshanks requested a review from a team as a code owner September 14, 2024 13:09
Copy link

netlify bot commented Sep 14, 2024

Deploy Preview for conda-forge-previews ready!

Name Link
🔨 Latest commit 31611d8
🔍 Latest deploy log https://app.netlify.com/sites/conda-forge-previews/deploys/66e58af049874400085da389
😎 Deploy Preview https://deploy-preview-2297--conda-forge-previews.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@jaimergp
Copy link
Member

jaimergp commented Sep 15, 2024

There have been several iterations in Azure (see microsoft/azure-pipelines-agent#1270), so we had to combine different "skipper" formulas to make sure we were not triggering CI services in large migrations (e.g. admin-migrations).

So it's one of those things that might not hurt to keep around "just in case".

Edit: Yes, some of those issues can be closed. There's some info worth keeping in the docs though. Can you add some text about this comment?

@wshanks
Copy link
Author

wshanks commented Sep 16, 2024

Hmm, microsoft/azure-pipelines-agent#1270 is interesting. It highlights that the skipper phrases only work with Azure Pipelines for merge commits, not for PRs. For PRs, the CI is still triggered. Here is a recent PR from the bot that uses [skip ci]: conda-forge/adversarial-robustness-toolbox-feedstock#50. We can see that Azure Pipelines still runs on conda-forge/adversarial-robustness-toolbox-feedstock@3a19536 while the merge commit conda-forge/adversarial-robustness-toolbox-feedstock@0be9377 does not. The GitHub Actions cf linter does not run on the PR though. As a test, I opened a PR using ***NO_CI*** in the title and commit message in conda-forge/ibm-platform-services-feedstock#63. In this case, both the Azure Pipelines and GitHub Actions jobs ran. This agrees with conda-forge/staged-recipes#1148 (comment) that you linked to.

So I don't see a case where ***NO_CI*** does something that [skip ci] does not do. I will leave it though if you think that is best. If we don't remove the ***NO_CI*** suggestion, we can change this PR to mention that the phrase only works on merge commits and not PRs and remove the old links. Is that what you would prefer?

I am not sure if CI running on PRs is what conda-forge wants or just a consequence of Azure Pipelines' design decision. conda-forge could add its own rule into its Azure Pipelines templates to skip PRs to save some CI time like numpy did (numpy/numpy#21879 (comment)).

@jaimergp
Copy link
Member

The GitHub Actions cf linter does not run on the PR though.

Uhh yea, that's not great. @beckermr, what do you think about this? Does it interact with the "mergeability" checks?

I am not sure if CI running on PRs is what conda-forge wants or just a consequence of Azure Pipelines' design decision. conda-forge could add its own rule into its Azure Pipelines templates to skip PRs to save some CI time like numpy did (numpy/numpy#21879 (comment)).

I think it's mostly an AZP limitation. This logic you linked to could be added to the conda-smithy templates to mitigate this surprising behaviour, but for sure we should document this clearly now so it's not buried in an issue somewhere.

@beckermr
Copy link
Member

I believe this is the intended behavior for the linter. If it sees [ci skip] it skips ci.

@jaimergp
Copy link
Member

jaimergp commented Oct 3, 2024

Thanks @wshanks, ended up adding conda-forge/conda-smithy#2077 and conda-forge/staged-recipes#27765 for Azure via your suggestion.

@@ -445,7 +445,7 @@ repo [README.md](https://github.com/conda-forge/webservices-dispatch-action) for

### Skipping CI builds

To skip a CI build for a given commit, put `[ci skip] ***NO_CI***` in the commit message.
To skip a CI build for a given commit, put `[ci skip]` in the commit message.

:::note[Related links]
Copy link
Member

Choose a reason for hiding this comment

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

We can remove this admonition now, I think.

@jaimergp jaimergp linked an issue Oct 3, 2024 that may be closed by this pull request
@jaimergp jaimergp mentioned this pull request Oct 3, 2024
@jakirkham
Copy link
Member

jakirkham commented Oct 3, 2024

Originally [ci skip] and [skip ci] worked for all CI providers (though I think Travis only liked one of those and forget what it was) and we added it to the linter

As we have moved from the original setup of Travis, CircleCI and AppVeyor through various additions and removals we have reached the current state of Azure, Travis and GitHub Actions (leveraged directly or using some other self-hosted runner solution)

Have forgotten some of the nuances of the different systems in between and how they handled skip notes. That said, [ci skip] and [skip ci] are pretty standard on most CI providers. So we have tried to make sure that works somehow. Thought Azure did some work on supporting this somewhere, but it has been a while since I looked and there are probably caveats

Since sometimes CI providers have not consistently done what we have wanted. For a time we added logic to fast cancel jobs in certain conditions (not the latest build for a PR or commit, recipe has a skip: true condition, PR doesn't merge cleanly into base branch, etc.) as well as test on merge commits consistently. Think as systems changed we dropped that logic. We could bring something like that back if it would improve user experience

@beckermr
Copy link
Member

beckermr commented Oct 3, 2024

Thanks @jakirkham. We just merged a pr to fast finish prs with skip messages for azure in smithy. To be honest it seems like we should just keep the logic around no matter what given the turnover over the years.

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

Successfully merging this pull request may close these issues.

abort builds with [skip ci]/etc
4 participants