add workflows for suggestions on PRs

[timo-a]

No description provided.

[pjfanning]

@timo-a how many PRs are you going to create? You already have #1347.

I'm getting super super suspicious right now.

[pjfanning]

-1 from me - see my concerns on #1347

[timo-a]

@timo-a how many PRs are you going to create? You already have #1347.

I'm getting super super suspicious right now.

Sorry for the confusion 😄
I tried to change the base of the original PR (which grew from origin/master) to origin/2.19 but that loaded a lot of commits, so I felt it would be cleaner to start anew with a PR from origin/2.19.
I could have rebased and force pushed the branch from my original PR, but since that branch was master instead of a dedicated feature branch I decided against that.

I have since addressed this on the original PR, but since you left a second comment there before I could press "comment" and I wanted to address the second one as well it took some time.

If we definitely won't merge into master then #1347 can be closed, as far as I am concerned.

[cowtowncoder]

@timo-a Quick question: is this still the change to ensure order of import statements for PRs? Asking since there are refs to "suggestions" and not sure what those would mean.

If it's just for that reformatting, I am more positive about this than @pjfanning, although I do agree that GH actions are very security sensitive things.

[pjfanning]

@cowtowncoder The pom.xml code uses OpenRewrite to reorder the imports. I have no problem using OpenRewrite generally but this change requires OpenRewrite to use a custom lib to find the code to do the ordering.

              <dependency>
                <groupId>io.github.timo-a</groupId>
                <artifactId>rewrite-recipe-starter</artifactId>
                <version>0.4.0</version>
              </dependency>

I think at a minimum that this should be a lib under Jackson control - ie that we fork this and build and publish this ourselves.

Then there is also the 2nd workflow which is unrelated to import order - comment-pr.yml - that sends all our code to a Google owned Github Action that adds comments all over the PR making suggestions.

[cowtowncoder]

@pjfanning Ah. Yes, the point about custom code is important wrt security. OpenRewrite I had heard about before (and being contacted about, via Tidelift community), just never had time to look into it.
But sounded/sounds like a useful and solid tool in itself.
Custom extensions, with floating version are definitely potential security concerns, esp. when using floating labels for versioning (i.e. code can change with no change to project using the tool)

EDIT: actually since extension is via Maven, published version is immutable. So at least that cannot change without PR to repos affected.

And the suggestions... yeah no. I am not a fan of "useful" suggestions as the baseline; re-ordering is something I'd like, AI (?) generated suggestions at most as PoC of some kind, opt-in/on-request.

[timtebeek]

Hi all; figured I'd chime in here with a bit of context, and a potential way forward. I'd spotted this PR linked on the PR at langchain4j, after we've already been using this on the OpenRewrite GitHub org a little over half a year now. The suggested code changes are strictly coming from predictable recipes; there's no AI involved when running OpenRewrite. We similarly have recipes to migrate Jackson v1 to v2, maintained at openrewrite/rewrite-jackson.

In terms of security the workflow is intentionally split in two parts; one that runs recipes on the changed code in the PR, and a second on that runs off the master branch to post suggestion comments on the PR. That way the token isn't available to any unsafe contributed code, and you can further restrict the token permissions if you'd like:

• https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/controlling-permissions-for-github_token
• https://docs.github.com/en/actions/security-for-github-actions/security-guides/automatic-token-authentication

In terms of floating versions & external dependencies: perhaps the Jackson related recipes under io.github.timo-a:rewrite-recipe-starter can move to openrewrite/rewrite-jackson, or indeed the FasterXML GitHub org. Under the OpenRewrite org I can help review, maintain & release them with each new OpenRewrite release, and expand on those recipes with new releases of Jackson. But I'll try to help all the same if those end up under the FasterXML org, it just won't automatically get picked up in those cases.

From our perspective we've seen this workflow work really well: repeat contributors get near immediate feedback on small improvements they could make to their PR. First time contributors get the same feedback after an initial approval to run the recipes. That way I mostly see PRs with suggestions already applied, and no longer have to repeatedly call out conventions that go beyond what a linter would enforce. For inspiration you can take a look at the recipes we enforce across our 50+ repositories.

I hope that's helpful to the discussion here; happy to work with you all to get the same benefits applied here, or other recipes that might help your users.

[cowtowncoder]

Thank you @timtebeek -- this is really helpful.

+1 for Jackson-related recipes to moving under OpenRewrite org, that seems like a good move to me.

Quick question on suggestions: do you have a link to example(s) of suggestions done to PRs in other projects? I think seeing things in action would help alleviate concerns about noise that I have (based on past experiences with different tools -- which is not fair baseline, of course).

[timtebeek]

Sure! Here's a screenshot of what those automated suggestions look like in practice:

They're taken from pull request openrewrite/rewrite-migrate-java#421, where good, detailed work was done, that merely had some conventions not applied before a merge. Rather than trouble the reviewer or contributor to open their IDE for these small adjustments, we just post them as suggestions that can be one-click-applied from GitHub. We've also done a write up of this approach on the Moderne blog.

The standards enforced can of course be adjusted per project; We started with suggesting license headers to be added to new files (often forgotten), as well as some testing best practices. Imports are another such case as seen here, but there's some 2800 other recipes to choose from still. :)

[cowtowncoder]

Thank you @timtebeek. So, on suggestions, I think my concern would be wrt. selection of things to suggests: there being 2 often conflicting goals:

1. Optimal choice for projects ("things we want and only those things")
2. Low-effort maintenance ("let's just use defaults")

In our particular case, for example, one example mentioned (adding of license comment boilerplate) happens to be something we would not want. Same is true for project-specific indentation rules or -- relevant here -- import order.

And so what I'd guess as the approach -- start with defaults, add overrides -- may not be particular low effort. But the opposite: start with no suggestions, add what makes sense, could be challenging too - how do you find ones you want out of 2800?

So. To some degree, while I think the idea of suggestions is fundamentally positive, it might be tricky to work through. My specific concerns are based on seeing the effort needed by some projects (well, in a way even Jackson) to add various overrides (exclusions) for warnings emitted by FindBugs (et al), or even just IDEs, for things that are often code smell except in specific case (... of which are way more common than assumed :) ).

So... WDYT?

[timtebeek]

hi @cowtowncoder ; the way we've used is to find a core set of recipes whose results we like; then apply them once across the code base, and enforce them going forward. I can be involved with suggesting those sets of recipes if you'd like, and I'm sure @timo-a has a few recipes that fit the project well too. Then gradually you can expand that list of recipes; each time clearing out all existing, before enforcing them going forward.

For example, with LangChain4j (and others before it) we've started with some testing best practices of consistently adopting AssertJ:

• Apply additional AssertJ best practices since #673 langchain4j/langchain4j#1904

The nice thing there is that you have great code coverage on the suggested changes, and there's immediate value in more expressive assertions and better error messages on failures. We can start elsewhere of course, as we have similar best practices for JUnit Jupiter, SLF4J and others.

Such a gentle introduction also ensures each PR that clears out existing issues is easy to review, and you can preview the results quickly through the Moderne platform by following this link: https://app.moderne.io/devcenter/FasterXML; as an example the AssertJ best practices would produce these changes: https://app.moderne.io/results/Al4Cc4R2N
Note that I'm not saying to immediately apply all of those, but it's an example to give you a sense of maintaining the code at scale.