Skip to content

Conversation

jhen0409
Copy link
Contributor

The pull_request_target event is similar to pull_request but allows specific activity types. In my case, I want to limit the trigger to [opened, synchronize].

jhen0409 added a commit to jhen0409/github-actions-report-lcov that referenced this pull request Jun 17, 2022
@balaji-srin
Copy link

I just created #25 . See my reasonings in the PR. Will the changes in this PR work the same way as mine? Because the event is always non-null (non-empty) when the work flow is triggered? So we might as well drop the check on the event name itself?

@balaji-srin
Copy link

@jhen0409 By the way, you have not uploaded the generated files under dist folder. My understanding is that that we need those files to for the github actions to work. My PRs all contain those files. May be I have misunderstood something?

@jhen0409
Copy link
Contributor Author

jhen0409 commented Jun 17, 2022

@jhen0409 By the way, you have not uploaded the generated files under dist folder. My understanding is that that we need those files to for the github actions to work. My PRs all contain those files. May be I have misunderstood something?

I hope the PR can be reduced the review noises, so I expected it can be build when release new version.

I just created #25 . See my reasonings in the PR. Will the changes in this PR work the same way as mine? Because the event is always non-null (non-empty) when the work flow is triggered? So we might as well drop the check on the event name itself?

I saw #25 I think it will broke the workflow on another trigger types, because it not always have pr number on every trigger types.

@balaji-srin
Copy link

I hope the PR can be reduced the review noises, so I expected it can be build when release new version.

Ah got it.

I saw #25 I think it will broke the workflow on another trigger types, because it not always have pr number on every trigger types.

I dont think I understand the comment. But I think you prefer to get the PR number for ever comment. Is that correct?

@jhen0409
Copy link
Contributor Author

I dont think I understand the comment. But I think you prefer to get the PR number for ever comment. Is that correct?

I mean github.context.payload.pull_request, it doesn't exist on trigger types like push. In case if I just want to update artifact without create comment, I will use the trigger type like that.

@zgosalvez
Copy link
Owner

Hello everyone. I just want to make sure I'm processing the right PR. So in lieu of this PR, #25 can be merged and this can be closed?

@jhen0409
Copy link
Contributor Author

Hello everyone. I just want to make sure I'm processing the right PR. So in lieu of this PR, #25 can be merged and this can be closed?

It might depend on the use case like I saw in #22 (comment).

However, these more trigger types may includes the payload. The best way probably is skip create comment when the payload is empty.

@balaji-srin
Copy link

From my side, I dont mind creation of multiple comments. Creation of a comment is a minor action and it does not take long. And I can also manually delete comments that are not needed. But this is just my use case.

The primary goal of #25 is that if I update my PR with more unit tests and the coverage improves, I want it to show up as a comment. If that is achieved in a better way, I will be happy to accept the solution.

@balaji-srin
Copy link

Note the PR #28 . This allows us to skip creating multiple comments. :-)

@zgosalvez
Copy link
Owner

Thank you for your patience and contribution. I'm kind of lost in the conversation but I've decided to merge this relatively straightforward PR. Thank you again!

@zgosalvez zgosalvez merged commit 54da59f into zgosalvez:main Nov 27, 2022
pavelsaman pushed a commit to pavelsaman/github-actions-report-lcov that referenced this pull request Feb 6, 2024
Bumps [@actions/artifact](https://github.com/actions/toolkit/tree/HEAD/packages/artifact) from 2.1.0 to 2.1.1.
- [Changelog](https://github.com/actions/toolkit/blob/main/packages/artifact/RELEASES.md)
- [Commits](https://github.com/actions/toolkit/commits/@actions/[email protected]/packages/artifact)

---
updated-dependencies:
- dependency-name: "@actions/artifact"
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants