search

openrewrite / rewrite-github-actions

OpenRewrite recipes for performing GitHub action hygiene and migration tasks.
Apache License 2.0
9 stars 9 forks source link

feat: add ActionsSetupJavaAdoptOpenj9ToIBMSemeru recipe #73

Closed yeikel closed 1 year ago

yeikel commented 1 year ago

Closes #70

yeikel commented 1 year ago

@sambsnyd Do you have any additional feedback?

@timtebeek, your feedback would be appreciated as well

Thanks!

timtebeek commented 1 year ago

Looks good to me, although I would have a made a few different choices perhaps. Are these the only two such recipes you expect?

Otherwise we might optimize a bit by for instance not having two explicit recipes with a shared configurable visitor, but one configurable recipe used twice from yaml. And for the tests you could consider using parameterized tests with """... distribution: %s ...""".formatted(distBefore) & """... distribution: %s ...""".formatted(distAfter). But that's not a change I'm going to ask you to make now with only two recipes, especially as this has been lying around for a few weeks by now. :)

Anything you'd like to see changed before a merge @sambsnyd ?

yeikel commented 1 year ago

Looks good to me, although I would have a made a few different choices perhaps. Are these the only two such recipes you expect?

Otherwise we might optimize a bit by for instance not having two explicit recipes with a shared configurable visitor, but one configurable recipe used twice from yaml. And for the tests you could consider using parameterized tests with """... distribution: %s ...""".formatted(distBefore) & """... distribution: %s ...""".formatted(distAfter). But that's not a change I'm going to ask you to make now with only two recipes, especially as this has been lying around for a few weeks by now. :)

Anything you'd like to see changed before a merge @sambsnyd ?

Are you saying that perhaps we should create a generic recipe MigrateJavaDistribution that takes distBefore and distAfter as arguments? Then create the two definitions as YAML recipes instead?

I am not aware of any other migration recipe impacting the distribution but it is possible that we might in the future

If so, that's a good feedback. I'd be open to make that change

timtebeek commented 1 year ago

Are you saying that perhaps we should create a generic recipe MigrateJavaDistribution that takes distBefore and distAfter as arguments? Then create the two definitions as YAML recipes instead?

I am not aware of any other migration recipe impacting the distribution but it is possible that we might in the future

If so, that's a good feedback. I'd be open to make that change

That's what I would do if there's ever a third; but like I said no need to make that change now; We can also add it as a comment on the visitor as a possible future refactoring. Especially if we only expect two for the foreseeable future.

timtebeek commented 1 year ago

Thanks again @yeikel !