Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Enable parallelism #213

Merged
merged 3 commits into from
Mar 28, 2024
Merged

Enable parallelism #213

merged 3 commits into from
Mar 28, 2024

Conversation

jnooree
Copy link
Contributor

@jnooree jnooree commented Mar 20, 2024

See cpp-linter/cpp-linter#92 for the related CLI updates.

@2bndy5
Copy link
Collaborator

2bndy5 commented Mar 21, 2024

FYI, dependabot will submit a PR that will update the CLI pkg when we release the changes in cpp-linter/cpp-linter#92. Until then we have other testing that we're trying to get done, so this will be in idle for the meantime.

@shenxianpeng shenxianpeng added the enhancement New feature or request label Mar 22, 2024
@shenxianpeng
Copy link
Collaborator

shenxianpeng commented Mar 27, 2024

@2bndy5 I think we should publish a patch release of the current draft release (backlog). Later after this PR and cpp-linter version bump to v1.8.0, we should publish a minor release. so split release can ensure stability and ease of control. your throughs?

@2bndy5
Copy link
Collaborator

2bndy5 commented Mar 27, 2024

Yeah, we can do a patch release right now. I have no objection to that.

As for parallelism, it should definitely be a minor version bump. I just did the same bump for cpp-linter.


Once dependabot submits the PR for cpp-linter v1.8.0, I'll be rebasing this branch and supplementing it with doc info.

@2bndy5
Copy link
Collaborator

2bndy5 commented Mar 28, 2024

Ok, I force-pushed a docs-related commit after rebasing the branch.

I think the self-test CI failed again because the PR is coming from a public fork, thus the token permission is inadequate to post a thread comment. This is what the pull_request_target event is supposed to be for, but I don't understand how it would help test the proposed changes.

ERROR:CPP Linter:response returned 403 from POST https://api.github.com/repos/cpp-linter/cpp-linter-action/issues/213/comments with message: {"message":"Resource not accessible by integration","documentation_url":"https://docs.github.com/rest/issues/comments#create-an-issue-comment"}

This message is not entirely informative, but that's all we get from github REST API 403 response.

@shenxianpeng
Copy link
Collaborator

Introduce pull_request_target should work. the current failure of post comments shouldn't affect anything.

@2bndy5
Copy link
Collaborator

2bndy5 commented Mar 28, 2024

the current failure of post comments shouldn't affect anything

Agreed. I don't see a good workaround right now. I added the pull_request_target trigger, but it won't take affect til merged to main.

@2bndy5 2bndy5 merged commit 065b5ba into cpp-linter:main Mar 28, 2024
29 of 32 checks passed
@shenxianpeng
Copy link
Collaborator

pull_request_target and pull_request are more confusing now. Btw, self test run success after the merge.

@2bndy5
Copy link
Collaborator

2bndy5 commented Mar 28, 2024

It is designed to let a "known good" CI workflow (the target branch's config) run changes in the PR. But, it ignores any incoming changes in the CI workflows config.

There is no way to allow comments to be posted using PR changes in a CI workflow from a public fork. That is the exact security risk that pull_request_target is designed to avoid.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants