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.

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

Improvements for DefaultGrailsPlugin: set delegation strategy before … #12892

Closed
wants to merge 2 commits into from

Conversation

rainboyan
Copy link
Contributor

…calling doWithSpring Closure

If Grails plugin has defined invokeMethod, an error will occur when doWithSpring is executed

Forward port of PR#12891 to 4.1.x

…calling doWithSpring Closure

If Grails plugin has defined invokeMethod, an error will occur when doWithSpring is executed
@puneetbehl
Copy link
Contributor

@rainboyan Thank you for the pull-request. Could you please look into why the tests are failing?

Also, I will pull the changes for upstream branches so you do not need to create another pull-request for 5.3.x branch.

@puneetbehl puneetbehl self-requested a review February 27, 2023 13:52
@rainboyan
Copy link
Contributor Author

@puneetbehl I run ./gradlew test on my local Mac, all tests passed, and I think the errors were not related my PR.

@puneetbehl
Copy link
Contributor

@rainboyan I am in the middle of something right now. I will take a look myself and get back to you. Thank you for the pull-request.

@rainboyan
Copy link
Contributor Author

@puneetbehl thank you for your help!

@rainboyan
Copy link
Contributor Author

@puneetbehl What's going on now? Any new releases planned for Grails 4?

@rainboyan
Copy link
Contributor Author

rainboyan commented Sep 26, 2023

@puneetbehl

Thank you for your pull-request. We only maintain the latest minor version which means the next release will be 4.1.3 and there will not be any 4.0.x release. So, I am closing this pull-request.

Waiting for your reply.

@puneetbehl
Copy link
Contributor

The Grails 4 has already reached the EOL. Can you please rebase your changes to Grails 5 at minimum?

Copy link
Contributor

@puneetbehl puneetbehl left a comment

Choose a reason for hiding this comment

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

Please rebase the changes on top of Grails 5.

@puneetbehl
Copy link
Contributor

puneetbehl commented Oct 5, 2023

Ideally, you should create your PR against the latest available stable version or default branch. Which should be the 6.0.x branch.

However, we are maintaining two major versions Grails 5 and 6. In this scenario, you should create your PR based on latest branch for the specific major version.

For example, if you are working on feature or bug-fix for version 5, create your branch based on the 5.3.x branch (or whichever branch corresponds to the latest version within the 5.x.x series).

In the above, none of these PR's are based on 5.3.x or 6.0.x.

@rainboyan rainboyan changed the base branch from 4.1.x to 5.3.x October 5, 2023 07:10
@rainboyan
Copy link
Contributor Author

@puneetbehl Thank you for your reply.

When I submitted this PR seven months ago, Grails 4 was still under maintenance, and you closed the other PRs at that time, leaving only the current PR for 4.1.x. But now, you're telling me that Grails 4 is EOL, which is something I'm very upset about.

As a user, of course, I also hope that the PR will be merged into Grails 4, which will benefit many legacy projects that cannot be upgraded to Grails 5/6.

As a contributor, this was very disappointing, and it took seven months to reply that Grails 4 was no longer maintained. I can't understand this rash decision by the Grails management team.

If you think this PR is useful, I believe you have the way to deal with it, and it will be more appropriate for you to do it yourself, because you have the final decision on whether to merge or not, and choose the appropriate branch to merge. That's why I submitted multiple PRs in the first place, based on what I've seen before, it's often the case that PRs are submitted in a lower version but not merged into the next version.

@puneetbehl
Copy link
Contributor

I want to apologize for the delay in responding to your Pull Request (PR) and any frustration it may have caused. When you submitted your PR seven months ago, Grails 4 was still under maintenance. The recent EOL announcement for Grails 4 understandably upset you.

Your desire to have your PR merged into Grails 4 is reasonable, especially for legacy projects. The delay in our response was disappointing, and we acknowledge the miscommunication.

Your point about PRs submitted for lower versions not being merged into the next version is noted. We're committed to reevaluating your PR and finding a resolution that benefits the community.

We have the final say on merging, and we'll choose the appropriate branch for integration. We're dedicated to learning from this experience and improving our communication. I've created another PR #13150 which add your changes to Grails 5, and 6.

Your contributions are valued, and we're eager to collaborate to enhance the Grails framework for all users.

@puneetbehl puneetbehl closed this Oct 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants