Skip to content

Automatically review PRs with OpenRewrite recipe runs #673

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

Merged
merged 8 commits into from
Oct 11, 2024
Merged

Automatically review PRs with OpenRewrite recipe runs #673

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5ff9ea6
Automatically review PRs with OpenRewrite recipe runs
timtebeek Feb 24, 2024
225b860
Consistently use GITHUB_TOKEN
timtebeek Feb 28, 2024
74266ee
Merge branch 'main' into automatically-review-PRs
timtebeek Mar 5, 2024
8751ee5
Merge branch 'main' into automatically-review-PRs
timtebeek Mar 14, 2024
308b733
Add a comment about how recipes can be run locally
timtebeek Mar 14, 2024
2a0b633
Apply fixes since PR was opened
timtebeek Mar 14, 2024
d9fc9f2
Merge branch 'main' into automatically-review-PRs
timtebeek Jul 23, 2024
750e628
Merge branch 'main' into automatically-review-PRs
dliubarskyi Oct 11, 2024
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 }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This token you get for free with GitHub Actions, and by default allows write access to the PR. That's also why we should not execute any user contributed code in this PR, and why we have the split between receive and comment into two workflows, as recommended.

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: '.'
Comment on lines +52 to +56
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the part of the workflows which actually posts the comments; I've seen it work quite well in practice, but in very rare cases a suggestion can be off a tiny bit. If that ever becomes an issue then reviewdog is an alternative.

54 changes: 54 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,54 @@
# 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]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Runs are quick; around ~1 minute locally here, so I figured no harm in doing this for every sync of a PR. Can be changed to just opened if you want to limit the runs.

branches:
- main

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
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
6 changes: 3 additions & 3 deletions ...in4j-vertex-ai/src/test/java/dev/langchain4j/model/vertexai/VertexAiEmbeddingModelIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ void testTokensCountCalculationAndBatching() {
void testRandomSegments() {
List<TextSegment> segments = createRandomSegments(10, 100);

assertThat(segments.size()).isEqualTo(10);
assertThat(segments).hasSize(10);
for (TextSegment segment : segments) {
assertThat(segment.text()).hasSizeLessThan(100);
}
Expand Down Expand Up @@ -130,7 +130,7 @@ void testBatchingEmbeddings() {

List<Embedding> embeddings = model.embedAll(segments).content();

assertThat(embeddings.size()).isEqualTo(1234);
assertThat(embeddings).hasSize(1234);
}

@Test
Expand Down Expand Up @@ -158,7 +158,7 @@ void testBatchingEmbeddingsWithMaxSet() {

List<Embedding> embeddings = model.embedAll(segments).content();

assertThat(embeddings.size()).isEqualTo(1234);
assertThat(embeddings).hasSize(1234);
}

@Test
Expand Down
14 changes: 7 additions & 7 deletions langchain4j/src/test/java/dev/langchain4j/service/AiServicesBuilderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
import org.mockito.Mockito;
import org.mockito.Spy;

import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.assertj.core.api.AssertionsForClassTypes.assertThatExceptionOfType;
import static org.mockito.Mockito.mock;

/**
Expand All @@ -30,7 +30,7 @@ public void testRetrieverAndContentRetriever() {
});
ContentRetriever contentRetriever = mock(ContentRetriever.class);

assertThrows(IllegalConfigurationException.class, () -> {
assertThatExceptionOfType(IllegalConfigurationException.class).isThrownBy(() -> {
AiServices.builder(AiServices.class)
.retriever(retriever)
.contentRetriever(contentRetriever)
Expand All @@ -46,7 +46,7 @@ public void testRetrieverAndRetrievalAugmentor() {
});
RetrievalAugmentor retrievalAugmentor = mock(RetrievalAugmentor.class);

assertThrows(IllegalConfigurationException.class, () -> {
assertThatExceptionOfType(IllegalConfigurationException.class).isThrownBy(() -> {
AiServices.builder(AiServices.class)
.retriever(retriever)
.retrievalAugmentor(retrievalAugmentor)
Expand All @@ -59,7 +59,7 @@ public void testContentRetrieverAndRetrievalAugmentor() {
ContentRetriever contentRetriever = mock(ContentRetriever.class);
RetrievalAugmentor retrievalAugmentor = mock(RetrievalAugmentor.class);

assertThrows(IllegalConfigurationException.class, () -> {
assertThatExceptionOfType(IllegalConfigurationException.class).isThrownBy(() -> {
AiServices.builder(AiServices.class)
.contentRetriever(contentRetriever)
.retrievalAugmentor(retrievalAugmentor)
Expand All @@ -72,7 +72,7 @@ public void testContentRetrieverAndRetriever() {
Retriever retriever = mock(Retriever.class);
ContentRetriever contentRetriever = mock(ContentRetriever.class);

assertThrows(IllegalConfigurationException.class, () -> {
assertThatExceptionOfType(IllegalConfigurationException.class).isThrownBy(() -> {
AiServices.builder(AiServices.class)
.contentRetriever(contentRetriever)
.retriever(retriever)
Expand All @@ -85,7 +85,7 @@ public void testRetrievalAugmentorAndRetriever() {
Retriever retriever = mock(Retriever.class);
RetrievalAugmentor retrievalAugmentor = mock(RetrievalAugmentor.class);

assertThrows(IllegalConfigurationException.class, () -> {
assertThatExceptionOfType(IllegalConfigurationException.class).isThrownBy(() -> {
AiServices.builder(AiServices.class)
.retrievalAugmentor(retrievalAugmentor)
.retriever(retriever)
Expand All @@ -98,7 +98,7 @@ public void testRetrievalAugmentorAndContentRetriever() {
ContentRetriever contentRetriever = mock(ContentRetriever.class);
RetrievalAugmentor retrievalAugmentor = mock(RetrievalAugmentor.class);

assertThrows(IllegalConfigurationException.class, () -> {
assertThatExceptionOfType(IllegalConfigurationException.class).isThrownBy(() -> {
AiServices.builder(AiServices.class)
.retrievalAugmentor(retrievalAugmentor)
.contentRetriever(contentRetriever)
Expand Down
29 changes: 29 additions & 0 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,5 +170,34 @@
</plugins>
</reporting>

<profiles>
<profile>
<id>openrewrite</id>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically we don't absolutely need a profile for the workflows to work; but by having a profile here we more easily can bump the versions of the plugin and dependency, and folks can run the same fixes locally with just:

mvn --activate-profiles openrewrite org.openrewrite.maven:rewrite-maven-plugin:run

Copy link

Choose a reason for hiding this comment

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

I think having a dedicated profile is the right way.
As maven documentation states "Under other circumstances, a slightly different dependency set will be required, (...)" and that's the situation here.

Maybe chaining the plugin execution to a specific build step (verify) would be even more maven-ish.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! You mean to only add it to a build step in this particular profile? That could make sense indeed. I hadn't wanted to tie it into the lifecycle originally, as to not cause delays for folks, but within a profile that makes sense.

<!-- `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.23.1</version>
<configuration>
<activeRecipes>
<!--<recipe>org.openrewrite.java.OrderImports</recipe>-->
<recipe>org.openrewrite.java.testing.assertj.Assertj</recipe>
</activeRecipes>
<failOnDryRunResults>true</failOnDryRunResults>
</configuration>
<dependencies>
<dependency>
<groupId>org.openrewrite.recipe</groupId>
<artifactId>rewrite-testing-frameworks</artifactId>
<version>2.4.1</version>
</dependency>
</dependencies>
</plugin>
</plugins>
</build>
</profile>
</profiles>

</project>
Loading