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

add file-annotations option #59

Merged
merged 7 commits into from
Apr 28, 2022
Merged

Conversation

2bndy5
Copy link
Collaborator

@2bndy5 2bndy5 commented Apr 27, 2022

Add an option to enable/disable the use of file annotations. This includes changes to the README, so there might be a conflict with #57.

@2bndy5 2bndy5 added documentation Improvements or additions to documentation enhancement New feature or request labels Apr 27, 2022
@2bndy5 2bndy5 marked this pull request as ready for review April 27, 2022 17:21
@2bndy5
Copy link
Collaborator Author

2bndy5 commented Apr 27, 2022

I tested this option on the test repo. In a matrix that runs the action for all versions of clang-tools, only annotations (& thread comments) were enabled for v12. It worked as expected.

@shenxianpeng
Copy link
Collaborator

Do you think we need to update the YAML example in README to help users avoid adding duplicate annotaions?

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Apr 28, 2022

Well, following the discussion in #58, it seems that there are several ways to trigger duplicate annotations.
I think I'd propose the example snippets demonstrate using specific event types rather than using this new option.

on:
  pull_request:
    types: [opened, reopened]  # let PR-synchronize events be handled by push events
  push:

The current examples use paths-ignore which is very specific to this repo:

on:
  push:
    paths-ignore: "docs/**"
  pull_request:
    paths-ignore: "docs/**"

@shenxianpeng
Copy link
Collaborator

I like your propose 👍

@sonarcloud
Copy link

sonarcloud bot commented Apr 28, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Apr 28, 2022

I accidentally rebased on an older commit in master, so the commits in #57 now show in this branch's history. Please squash and merge this when ready.

@shenxianpeng
Copy link
Collaborator

OK. do you know why the PR action is queued https://github.com/shenxianpeng/test-cpp-linter-action/actions/runs/2236867491
is it related to the change below?

on:
  pull_request:
    types: [opened, reopened]  # let PR-synchronize events be handled by push events
  push:

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Apr 28, 2022

I have no idea. I tried canceling the run (hoping to manually re-start it), but I got a error message from github saying "Failed to cancel workflow." It doesn't seem to be related.

Sometimes, I see a runner in queue when waiting for a github hosted VM to become available, but it doesn't even say that much. 🤷🏼‍♂️

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Apr 28, 2022

It looks like a github problem. I see you closed and re-opened the PR which seemed to have expected results.

@shenxianpeng
Copy link
Collaborator

OK, I'm going to squash and merge.

@shenxianpeng shenxianpeng merged commit aaa3b1c into master Apr 28, 2022
@shenxianpeng shenxianpeng deleted the disable-annotations-option branch April 28, 2022 03:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants