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

Prevent tasks from running on Pull Request builds #800

Closed
gep13 opened this issue Feb 27, 2021 · 13 comments
Closed

Prevent tasks from running on Pull Request builds #800

gep13 opened this issue Feb 27, 2021 · 13 comments
Labels
Milestone

Comments

@gep13
Copy link
Member

gep13 commented Feb 27, 2021

Some tasks are executed during a Pull Request build, when they shouldn't be.

For example, pushing packages to NuGet/Chocolatey feeds shouldn't happen when running on a PR build. The packages should be created, but they shouldn't be published anywhere.

@gep13 gep13 added the Bug label Feb 27, 2021
@gep13 gep13 added this to the 2.2.1 milestone Feb 27, 2021
@gep13
Copy link
Member Author

gep13 commented Feb 27, 2021

The logic for identifying a PR is:

var headRef = context.BuildSystem().GitHubActions.Environment.Workflow.HeadRef;
        var branchRef = context.BuildSystem().GitHubActions.Environment.Workflow.Ref;

        IsPullRequest = !string.IsNullOrEmpty(headRef) && !string.IsNullOrEmpty(branchRef)
            && branchRef.IndexOf("refs/pull/") >= 0 && branchRef.IndexOf("/merge") >= 0;

However, in an example PR, the following values are in play:

               CI: true
             HOME: [NULL]
       GITHUB_REF: 
refs/heads/dependabot/nuget/develop/Microsoft.NET.Test.Sdk-16.9.1
       GITHUB_SHA: 861277b8e81f200042b5d9a358f5e74833c85d97
     GITHUB_ACTOR: dependabot[bot]
    GITHUB_ACTION: cake-buildcake-action
    GITHUB_RUN_ID: 604078695
   GITHUB_ACTIONS: true
  GITHUB_BASE_REF: 
  GITHUB_HEAD_REF: 
  GITHUB_WORKFLOW: Build
 GITHUB_WORKSPACE: D:\a\Cake.Json\Cake.Json
GITHUB_EVENT_NAME: push
GITHUB_EVENT_PATH: D:\a\_temp\_github_workflow\event.json
GITHUB_REPOSITORY: ***/Cake.Json
GITHUB_RUN_NUMBER: 15

@AdmiringWorm
Copy link
Member

@gep13 the build check you are linking to is not actually the pull request build, but rather a build that gets ran due to the push to a local branch.

I answered a similar (or actually the same question) here: cake-build/cake#3200

@AdmiringWorm
Copy link
Member

AdmiringWorm commented Feb 27, 2021

The failure you are seeing should be able to be resolved by a user when #775 is released.

EDIT: fixed issue link

@gep13
Copy link
Member Author

gep13 commented Feb 27, 2021

Hmmm.... thanks for pointing this out!

While the skip duplicate would prevent this issue, I also don't want any pre-release packages being pushed when it is a PR build (whether is it the PR branch itself or a local branch from same repository).

I am assuming that this can be fixed by preventing the build to only specific named branches on local repository?

@AdmiringWorm
Copy link
Member

I am assuming that this can be fixed by preventing the build to only specific named branches on local repository?

Indeed, but that is probably something that should be done in a workflow and not in Cake.Recipe itself.
I think the information we have available during the build is too limited to be able to do reliably in Cake.Recipe (would love to be wrong on that though).
If done in Cake.Recipe, we would possibly skip builds that should have been ran.

@gep13
Copy link
Member Author

gep13 commented Feb 27, 2021

Am I wrong in saying that we can use the event name, either from the environment variables, or by deserializing the event as done here: https://github.com/benfoster/o9d-guard/blob/main/build.cake and if it is a pull request event, don't do the publishing?

@AdmiringWorm
Copy link
Member

@gep13 not really necessary to check the event name.
There is code already available in the build provider class to check if it is a pull request or not by checking the git branch name (we don't use it when deciding to publish packages).
But there could be adding code to check the event name and fall back to the git branch parsing code already available.

It still wouldn't fix the error you saw, as it isn't considered a pull request since it is started on the repository branch itself, though.

@gep13
Copy link
Member Author

gep13 commented Feb 28, 2021

Ok, so now I am getting somewhere...

I was looking at this PR:

cake-contrib/Cake.Incubator#134

And the failing build took me through to this workflow:

https://github.com/cake-contrib/Cake.Incubator/pull/134/checks?check_run_id=1990873969

But this takes me to the push event build, rather than the pull request event build. The pull request event build didn't actually run, it was skipped.

image

The question that this leaves is that if I update the build configuration to limit builds to only master/develop/hotfix/release branches, will I still see the pull request event build? Going to need to play with this to confirm either way.

I think it comes down to the fact that I personally don't want pre-release packages to be published as a result of a dependabot push/pull request event. I don't think I want to go as far as limiting publishing to the GITHUB_ACTOR which could be an option:

GITHUB_ACTOR: dependabot[bot]

@AdmiringWorm
Copy link
Member

The question that this leaves is that if I update the build configuration to limit builds to only master/develop/hotfix/release branches, will I still see the pull request event build?

If you remove the if statement, then yes the pull request would still build.
These are two different events, and only the push event would be limited by the branch filter (either blacklist, or whitelist).

@AdmiringWorm
Copy link
Member

Did some digging around in the code, and even if everything gets limited we still have something that needs to be updated in Cake.Recipe itself.

The criteria for publishing don't take into account if the current build is a pull request or not:
https://github.com/cake-contrib/Cake.Recipe/blob/develop/Cake.Recipe/Content/packages.cake#L138-L141
https://github.com/cake-contrib/Cake.Recipe/blob/develop/Cake.Recipe/Content/packages.cake#L162-L165

so this is probably something that should be updated.

@gep13
Copy link
Member Author

gep13 commented Feb 28, 2021

Yip, sounds like a change is indeed required. Thanks for digging into this. I think there used to be a check, so this is perhaps a regression.

@AdmiringWorm
Copy link
Member

Yeah, either a regression or an oversight from back when we changed from only supporting AppVeyor to multiple build providers.

@gep13 gep13 changed the title Pull requests are not identified correctly when running on GitHub Actions Prevent tasks from running on Pull Request builds Mar 15, 2021
@gep13 gep13 closed this as completed Mar 15, 2021
gep13 added a commit that referenced this issue Mar 15, 2021
* hotfix/2.2.1:
  (#811) Add cake-recipe tag to nuspec
  (#800) Add IsPullRequest criteria to tasks
  (GH-793) error when vcsroot.branch is missing
  (GH-801) Update to try pushing chocolatey packages on Windows platform
  (#805) Fix string formatting syntax
@cake-contrib-bot
Copy link
Contributor

🎉 This issue has been resolved in version 2.2.1 🎉

The release is available on:

Your GitReleaseManager bot 📦🚀

gep13 added a commit to gep13/Cake.Recipe that referenced this issue Mar 15, 2021
* hotfix/2.2.1:
  (cake-contrib#811) Add cake-recipe tag to nuspec
  (cake-contrib#800) Add IsPullRequest criteria to tasks
  (cake-contribGH-793) error when vcsroot.branch is missing
  (cake-contribGH-801) Update to try pushing chocolatey packages on Windows platform
  (cake-contrib#805) Fix string formatting syntax
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants