skip_commit works, but builds still wait endlessly for appveyor to report status

[tooomm]

As described here: https://www.appveyor.com/docs/appveyor-yml/
I implemented skip_commits: for *.md files and some others at https://github.com/Cockatrice/Cockatrice/tree/tooomm-appveyor_config

It works nicely and I successfully tested it for several cases in the last commits in that branch.

However, if you look at the PR there (2888), you see that there still is one AppVeyor check waiting at the bottom overview. Even though for each of the test commits there are correctly no AppVeyor builds triggered.
Problem there is that the "waiting for AppVeyor to report status" check won't change, and if a appveyor build is marked as required that pr can't get merged at all...

Please let me know what I can do. 👍

[IlyaFinkelshteyn]

You still should be able to forcibly merge a PR if you are repository administrator. Or you can ask repo administrator to do it.
Also you can update status using GitHub API yourself, please see this sample. It is created with assumption to be called from build, but you can easily to change it to run from your computer. However for me merging using repo administrator privileges seems to be more correct approach.

[tooomm]

You are describing a workaround, not a solution. :)

Is this behavior of pending build checks ("Waiting for status to be reported ") expected, even though a build never triggered?

[IlyaFinkelshteyn]

We do not set this status. We set status to pending when build is good to go, after all filtering and with Waiting for AppVeyor build to complete message. This one is probably set by GitHub. What solution do you think we can do? There is no such status as cancelled in API, using any of existing does not look technically correct...

[tooomm]

Thanks for coming back to me that quickly! 👍

To understand you correctly, is this what you're saying?
Do the steps look more or less like this:

• GitHub triggers AppVeyor build (push to branch or pr)
• AppVeyor sets pending status for GitHub and checks the commit
• if the source commit is matching the filter criteria from appveyor.yml, no build is created, bail
• GitHub status check doesn't get altered again

[IlyaFinkelshteyn]

No... Appveyor does not set pending status before checking filtering criteria, it is being set only if build is really going to be built. And we set pending status with different description, this does not look like status created by us.

[tooomm]

No... Appveyor does not set pending status before checking filtering criteria, it is being set only if build is really going to be built.

Great, so because of commit filtering, there should never start any AppVeyor build in my example. That's how I understood and expected the feature to work. 👍
Matching filter criteria --> no build --> no status is set --> no status check is displayed in GitHub
In my example it's somehow the case though and there is still that weird pending status for no reason. The AppVeyor page also doesn't show any builds for the later commits (and the former successfully build).

And we set pending status with different description, this does not look like status created by us.

Thanks, so it looks like there is something unusual going on and I might ran into a bug.
That's why I got confused and opened the issue here in the first place.

Is there a way to check what triggered that status?

---

Edit: To make it more clear, that's how it looks like: (I figuered that other people might not see it...)

Edit2: After posting the pic and some more thoughts this might be a GitHub status check issue. As described before we enforce some checks as required in GitHub.
It looks like this makes GitHub expect these builds to always happen, therefore the "waiting for pending" status unrelated if there is or going to be a build at all.
See: https://discuss.circleci.com/t/not-able-to-merge-pull-request-containing-a-commit-with-ci-skip/4103

[IlyaFinkelshteyn]

Yes, this is exactly what I was thinking, I just did not find any "official" confirmation online. So we could reset this status if build if filtered out, but to what? As I said, all statuses available in API, are not technically correct for this case. It should be something like cancelled or better skipped...

[tooomm]

Yes, this is exactly what I was thinking, I just did not find any "official" confirmation online.

This special case is indeed not mentioned in their explanation or help pages.

It should be something like cancelled or better skipped...

While skipped might be a perfect fit, it is a very narrow and special term the API might not support soon. cancelled on the other hand is a repeatedly requested status and GitHub should be more likely to implement that.
It would be ok information wise, but sadly doesn't help with Required builds at all. After all, cancelled is still different than success... which a protected branch with required relies on.

As I said, all statuses available in API, are not technically correct for this case.

What do you think about this:

As your link to the GitHub API shows, statuses can contain additional optional information like description, target_url etc.
Nothing new and AppVeyor uses that already. 👍

For example, a success build shows this in GitHub right now:

• continuous-integration/appveyor/pr — AppVeyor build succeeded

Would it be possible to send a new status/description/target_url combination for builds that match commit filter criteria and are skipped by AppVeyor?
It could be the same success status, but another description like

• continuous-integration/appveyor/pr — AppVeyor build skipped
• continuous-integration/appveyor/pr — skipped build as configured
• continuous-integration/appveyor/pr — skip commit as in appveyor.yml

target_url could be the source config file like "https://github.com/user/repo/appveyor.yml".
If no suitable and helpful target_url can be found, description should contain more details like in the last example above.
Or even no "Details" link at all would be an improvement, as long as the build "passes" and the description is properly explaining what's going on/what's different.

Then it works perfectly fine with Required status checks.
People don't want to turn them off just because they use the commit filtering feature of ApvVeyor.
I'm quite surprised that nobody complained about this before. Haha. :)

[IlyaFinkelshteyn]

But with this people can cheat repo owners by simple adding [skip ci] into commit message, and make it merge-able. Cancelled status with proper description would let repo owner to consciously force merge using admin rights.

[tooomm]

But with this people can cheat repo owners by simple adding [skip ci] into commit message, and make it merge-able.

I can't follow you here... what would be the difference?
People can add [skip ci] to their commits right now, too.

Cancelled status with proper description would let repo owner to consciously force merge using admin rights.

That also means, that for many minor changes you have to have admin rights to merge them.
And just very few people in a repo have that. Some active repos might be driven by contributors, not the actual owner and possibly only admin.
Especially with files like readme being ignored with skip_commit this forces a admin action for every low impact change!

Another general improvement unrelated to the status type itself:
Instead showing no info at the AppVeyor page itself, it would be very helpful to display that specific "triggered" build, but tell the user that it never started and got skipped because of its appveyor.yml config.
Same as you show that a build got canceled because an other commit got pushed to the same branch/pr for example:

The Details link within a PR at GitHub could link to that build and people would see and can check what's going on.

[IlyaFinkelshteyn]

If people add [skip ci] to their commits right now, commits will not become merge-able. If we report success, they will become merge-able which will open possibility to merge bad commit.
Regarding second question, we are working on how to better inform customers about skipped builds.

[tooomm]

Well, everybody should properly check what he is merging before doing so. :)
And he should know about the CI checks and their config...

So as it stands right now, skip_commit and similar AppVeyor configurations are not compatible and useful at all with GitHub protected branches and their required status checks.

Regarding second question, we are working on how to better inform customers about skipped builds.

Happy to hear that. 👍

Hmm, another thought:
If we would do the file check on our own in a separate script right at beginning? (install scripts for example)
Can we finish a AppVeyor build early and return a custom value, to make AppVeyor abort and report passed?

[IlyaFinkelshteyn]

Sure, I made a sample for you.

Please note that I made it check that *.md and only *.md files changed, to prevent people to make build "green" by simple adding some change to *.md file, while in reality updating something important. I strongly recommend to follow this practice.

[tooomm]

Sure, I made a sample for you.

Awesome. Had no time yet to check it out! I'm sorry.

Please note that I made it check that *.md and only *.md files changed, to prevent people to make build "green" by simple adding some change to *.md file, while in reality updating something important. I strongly recommend to follow this practice.

That is a smart move and seems to be very important! 👍

@IlyaFinkelshteyn Why did you use an api call to get the changed files instead using git diff command?
Is there a particular benefit? I wasn't even aware one can do that. Cool :)

Am I right, that I can call it like this from within appveyor config if i place your example file within our repo?
Is init the right place to call it? Regarding the build pipeline it looks to be the earliest possible place to run a custom script:

# scripts that are called at very beginning, before repo cloning
init:
  - ps: .\scripts\GitHubCheckFiles.ps1

Which minimal scopes are needed for the token to properly access the api in this regard? There are a ton of options in the github settings...

[IlyaFinkelshteyn]

Honestly I did not think of git diff command. But even if you can make it work, parsing command line output is much less reliable than using built-in PowerShell support for JSON object returned by GitHub API.

Yes, init is a good place. You can wrap it into .ps1 script or have the whole thing in the YAML as a script block. Just please use secure variable for access token.

repo scope should be enough.

[tooomm]

Just please use secure variable for access token.

Hmm, your docs are saying: secure variables are not decoded during Pull Request builds

Wouldn't that then prevent it from working for all pr's?
Appveyor runs mostly on pr's to check them... the idea is to prevent as many not useful appveyor runs as possible, not only the few from master after merging a .md-only change.
Or did I get it wrong?

I was wondering why we would have to use a token at all, but I think I found it in the api docs. The Appveyor server would get limited upon their IP most likely: Rate Limiting

For unauthenticated requests, the rate limit allows for up to 60 requests per hour. Unauthenticated requests are associated with the originating IP address, and not the user making requests.

Even though it looked like a promising and better solution first, I think the API approach isn't working here due to ip limits for unauthenticated requests on one hand, and authenticated requests being limited to non-pr builds on the other hand?

[IlyaFinkelshteyn]

You have an option to allow secure variable in pull requests. For the same repository only if project us open source and for any repository if project is private. Does it work for you?

[tooomm]

I'll give it a try. Have to tinker around - never did most of that before. :)
Also requires action from the repo admin. Will come back to you and let you know. Thanks 👍

[sschuberth]

I'm very surprised to see this ticket being open still after such a long time, and TBH I believe you guys are over-complicating things: If skip_commits matches, then the AppVeyor build is a no-op, and if doing the no-op succeeds (i.e. there is not a more fundamental error like wrong syntax in appveyor.yml) then the build should simply be marked as green to make a required PR check pass.

[sschuberth]

Anyway, the way we do this in Travis is

before_install:
  - if [ -z $(git diff --name-only --diff-filter=d $TRAVIS_COMMIT_RANGE | grep -v "\.md$") ]; then
        echo "Only Markdown files were modified, skipping CI run.";
        exit;
    fi

What would be the equivalent of the $TRAVIS_COMMIT_RANGE environment variable in AppVeyor? Something like $env:APPVEYOR_REPO_BRANCH..$env:APPVEYOR_PULL_REQUEST_HEAD_REPO_BRANCH?

[sschuberth]

Here's what I ended up doing:

https://github.com/oss-review-toolkit/ort/blob/5b41416c0757b9fea730ab6b4a64cae1c7c88beb/.appveyor.yml#L29-L35