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.

Sign up for GitHub

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

Jump to bottom

add workflows for suggestions on PRs #1347

Closed
wants to merge 1 commit into from
Closed

add workflows for suggestions on PRs #1347

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 56 additions & 0 deletions .github/workflows/comment-pr.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
# Description: This workflow is triggered when the `receive-pr` workflow completes to post suggestions on the PR.
# Since this pull request has write permissions on the target repo, we should **NOT** execute any untrusted code.
# https://securitylab.github.com/research/github-actions-preventing-pwn-requests/
---
name: comment-pr

on:
workflow_run:
workflows: ["receive-pr"]
types:
- completed

jobs:
post-suggestions:
# https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#running-a-workflow-based-on-the-conclusion-of-another-workflow
if: ${{ github.event.workflow_run.conclusion == 'success' }}
runs-on: ubuntu-latest
env:
# https://docs.github.com/en/actions/reference/authentication-in-a-workflow#permissions-for-the-github_token
ACCESS_TOKEN: ${{ secrets.GITHUB_TOKEN }}
timeout-minutes: 10
steps:
- uses: actions/checkout@v4
with:
ref: ${{github.event.workflow_run.head_branch}}
repository: ${{github.event.workflow_run.head_repository.full_name}}

# Download the patch
- uses: actions/download-artifact@v4
with:
name: patch
github-token: ${{ secrets.GITHUB_TOKEN }}
run-id: ${{ github.event.workflow_run.id }}
- name: Apply patch
run: |
git apply git-diff.patch --allow-empty
rm git-diff.patch

# Download the PR number
- uses: actions/download-artifact@v4
with:
name: pr_number
github-token: ${{ secrets.GITHUB_TOKEN }}
run-id: ${{ github.event.workflow_run.id }}
- name: Read pr_number.txt
run: |
PR_NUMBER=$(cat pr_number.txt)
echo "PR_NUMBER=$PR_NUMBER" >> $GITHUB_ENV
rm pr_number.txt

# Post suggestions as a comment on the PR
- uses: googleapis/code-suggester@v4
with:
command: review
pull_number: ${{ env.PR_NUMBER }}
git_dir: '.'
55 changes: 55 additions & 0 deletions .github/workflows/receive-pr.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
# Description: This workflow runs OpenRewrite recipes against opened pull request and upload the patch.
# Since this pull request receives untrusted code, we should **NOT** have any secrets in the environment.
# https://securitylab.github.com/research/github-actions-preventing-pwn-requests/
---
name: receive-pr

on:
pull_request:
types: [opened, synchronize]
branches:
- master
- 2.[0-9]+
- 3.[0-9]+
concurrency:
group: '${{ github.workflow }} @ ${{ github.ref }}'
cancel-in-progress: true

jobs:
upload-patch:
runs-on: ubuntu-latest
timeout-minutes: 10
steps:
- uses: actions/checkout@v4
with:
ref: ${{github.event.pull_request.head.ref}}
repository: ${{github.event.pull_request.head.repo.full_name}}
- uses: actions/setup-java@v4
with:
java-version: '21'
distribution: 'temurin'
cache: 'maven'

# Capture the PR number
# https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#using-data-from-the-triggering-workflow
- name: Create pr_number.txt
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not a good way to get the PR number - try first answer in https://stackoverflow.com/questions/59077079/how-to-get-pull-request-number-within-github-actions-workflow

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand, both my workflow and the first stackoverflow answer use github.event.number to get the PR number. In order for the other workflow to get the PR number, this workflow then uploads the number as an artifact which the other workflow can download. I don't think this can be done with environment variables as used in the stackoverflow answer.

run: echo "${{ github.event.number }}" > pr_number.txt
- uses: actions/upload-artifact@v4
with:
name: pr_number
path: pr_number.txt
- name: Remove pr_number.txt
run: rm -f pr_number.txt

# Execute recipes
- name: Apply OpenRewrite recipes
run: mvn --activate-profiles openrewrite org.openrewrite.maven:rewrite-maven-plugin:run

# Capture the diff
- name: Create patch
run: |
git diff | tee git-diff.patch
- uses: actions/upload-artifact@v4
with:
name: patch
path: git-diff.patch
33 changes: 33 additions & 0 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,4 +298,37 @@ tools.jackson.core.*;version=${project.version}
<scope>test</scope>
</dependency>
</dependencies>

<profiles>
<profile>
<id>openrewrite</id>
<!-- `mvn -P openrewrite org.openrewrite.maven:rewrite-maven-plugin:run` -->
<build>
<plugins>
<plugin>
<groupId>org.openrewrite.maven</groupId>
<artifactId>rewrite-maven-plugin</artifactId>
<version>5.41.0</version>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can't be used on the 2.x branches because it would cause a huge diff

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can try to put the info from the profile directly into the command in the workflow, I'll just have to figure out how to specify the version number there.
The advantage of having the profile is in the pom is that you can reuse it to apply the changes locally. On the other hand if all the information is in the workflow then the workflow does not depend on the pom and you do not run the risk that changes to the pom inadvertently change the workflow behavior so this might be safer.

Copy link
Member

@pjfanning pjfanning Oct 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have the imports ordered on master so we can afford to support this and have full reordering - it is just not a good idea to have this on the 2.x branches.

I still have my objections to using any build tooling that doesn't have multitudes of users and oodles of history to establish trust.

My preference is not to merge any of this. Low cost to benefit ratio to me.

If we insist on merging something like this - I would prefer to look at established tools to do this.

I code mainly in Scala and use Scalafmt a lot. But if someone came to me with a custom project and asked me to replace Scalafmt, I would need a load of convincing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your point. Openrewrite is still new. It is the best tool I know for automatic refactoring and by extension it can also reformat code to a custom style.
(tangent: I believe that it is a time sink to define a custom code style without also having tools to enforce it. Thus my main suggestion remains to use an established code style which can be easily enforced. But @cowtowncoder is not fan of comprehensive auto-formatters and the google formatter in particular and them having fun maintaining the project is ultimately more important to its success than adherence to a code style.)
I tried to see if I can use Openrewrite to enforce key aspects of the code style on PRs and thereby relieve the reviewer. I think such a workflow is really useful to fix common sonarqube findings on PRs in an industrial setting. However, I'm not sure if there are enough PRs on this project to justify adding that complexity to the project.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again trying not to cast stones - but your PR includes call to a lib that is under your control.
io.github.timo-a:rewrite-recipe-starter - this is a project with 0 stars, 0 forks

<configuration>
<activeRecipes>
<recipe>org.openrewrite.java.OrderImports</recipe>
</activeRecipes>
<activeStyles>
<style>io.github.timoa.misc.JacksonImportStyle</style>
</activeStyles>
<failOnDryRunResults>true</failOnDryRunResults>
</configuration>
<dependencies>
<dependency>
<groupId>io.github.timo-a</groupId>
<artifactId>rewrite-recipe-starter</artifactId>
<version>0.4.0</version>
</dependency>
</dependencies>
</plugin>
</plugins>
</build>
</profile>
</profiles>

</project>