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

comment-on-pr-number is ignored on push events #1177

Closed
ulrich-giraud opened this issue May 24, 2024 · 2 comments · Fixed by #1178
Closed

comment-on-pr-number is ignored on push events #1177

ulrich-giraud opened this issue May 24, 2024 · 2 comments · Fixed by #1178
Assignees
Labels
impact/regression Something that used to work, but is now broken kind/bug Some behavior is incorrect or out of spec p1 A bug severe enough to be the next item assigned to an engineer resolution/fixed This issue was fixed
Milestone

Comments

@ulrich-giraud
Copy link

What happened?

We upgraded our action version from 4.1.0 to 5.2.3 and noticed that we weren't getting any comment on PR when we merged.
Everything works correctly on pull_requests events but not anymore on push events and comment-on-number is set

Example

      - name: Retrieve current PR
        uses: jwalton/gh-find-current-pr@v1
        id: find_pr
        with:
          state: all

      - name: Run pulumi update
        uses: pulumi/actions@v5.2.3
        with:
          command: up
          stack-name: ${{ inputs.stack-name }}
          work-dir: ${{ inputs.work-dir }}
          cloud-url: '<redacted>'
          comment-on-pr-number: ${{ steps.find_pr.outputs.pr }}
          diff: true
          edit-pr-comment: false

Output of pulumi about

CLI
Version 3.116.0
Go Version go1.22.3
Go Compiler gc

Host
OS arch
Version "rolling"
Arch x86_64

Backend
Name ufmwk
URL
User ulrich
Organizations
Token type personal

Pulumi locates its logs in /tmp by default

Additional context

It sounds like the regression was introduced by this commit : ca33bed#diff-4fab5baaca5c14d2de62d8d2fceef376ddddcc8e9509d86cfa5643f51b89ce3dL114-R124

In fact, the code right now doesn't allow to comment on anything other than a pull_request event :

  if (config.commentOnPrNumber || config.commentOnPr) {
    const isPullRequest = context.payload.pull_request !== undefined;
    if (isPullRequest) {  // handlePullRequestMessage isn't called if context.payload.pull_request is not set
      core.debug(`Commenting on pull request`);
      invariant(config.githubToken, 'github-token is missing.');
      handlePullRequestMessage(config, projectName, output);
    }
  }

Contributing

Vote on this issue by adding a 👍 reaction.
To contribute a fix for this issue, leave a comment (and link to your pull request, if you've opened one already).

@ulrich-giraud ulrich-giraud added kind/bug Some behavior is incorrect or out of spec needs-triage Needs attention from the triage team labels May 24, 2024
@tgummerer tgummerer removed the needs-triage Needs attention from the triage team label May 24, 2024
@tgummerer tgummerer added the impact/regression Something that used to work, but is now broken label May 24, 2024
@tgummerer
Copy link
Contributor

Thanks for opening this issue! This does indeed look like a bug to me, and looks like an accidental regression in #813. I opened a PR to try and fix it in #1178.

@justinvp justinvp added this to the 0.105 milestone May 24, 2024
@justinvp justinvp added the p1 A bug severe enough to be the next item assigned to an engineer label May 24, 2024
@pulumi-bot pulumi-bot added the resolution/fixed This issue was fixed label May 28, 2024
@tgummerer
Copy link
Contributor

I just released v5.2.4 of the GitHub action, which should make this work again. https://github.com/pulumi/actions/releases/tag/v5.2.4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact/regression Something that used to work, but is now broken kind/bug Some behavior is incorrect or out of spec p1 A bug severe enough to be the next item assigned to an engineer resolution/fixed This issue was fixed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants