add workflows for suggestions on PRs #1347
Conversation
|
We don't typically start many PRs at master level. So I would prefer not to merge this. Most PRs start on the latest dev 2.x branch - currently 2.19. And the changes are then forward fit. |
|
I'm not making any accusations but I am super wary of any PRs that modify GitHub workflows. I am a strong believer that you need a much higher level of trust when it comes to PRs like this.
|
|
|
||
| # 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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| <plugin> | ||
| <groupId>org.openrewrite.maven</groupId> | ||
| <artifactId>rewrite-maven-plugin</artifactId> | ||
| <version>5.41.0</version> |
There was a problem hiding this comment.
This can't be used on the 2.x branches because it would cause a huge diff
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
I have created a second PR against the proper target branch: #1348.
I am not taking this personally, you are correct to be wary of changes regarding the workflow. I would even go further and say that such changes should be measured against a higher standard than trust towards a contributor or their verifiable GitHub commit history. They should be not be approved if the maintainers do not understand their contents. (For security but also sustainability, I am making no commitment to provide fixes if needed) I am happy to answer questions regarding the workflow changes. I also understand if the review takes some time or ultimately results in rejection of the PR. |
|
We only need one PR. |
No description provided.