Skip to content

Conversation

@jamesfredley
Copy link
Contributor

the servlet container should be selected
via --servlet
jetty none tomcat undertow

Selecting via features does not properly override ServletImpl

the servlet container should be selected
via --servlet
jetty      none       tomcat     undertow

Selecting via features does not properly override ServletImpl
@jamesfredley jamesfredley self-assigned this Aug 21, 2025
@jamesfredley jamesfredley moved this to In Progress in Apache Grails Aug 21, 2025
@jamesfredley jamesfredley added this to the grails:7.0.0-RC2 milestone Aug 21, 2025
@jamesfredley jamesfredley requested a review from matrei August 21, 2025 23:52
@jamesfredley jamesfredley mentioned this pull request Aug 21, 2025
12 tasks
Copy link
Contributor

@matrei matrei left a comment

Choose a reason for hiding this comment

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

Should we also simplify the condition in the shouldApply methods of SpringBoot{ServletContainer}Feature classes.

public boolean shouldApply(ApplicationType applicationType, Options options, Set<Feature> selectedFeatures) {
    return options.getServletImpl() == ServletImpl.JETTY;
}

Refactored the shouldApply method in SpringBootJettyFeature, SpringBootTomcatFeature, and SpringBootUndertowFeature to only check the servlet implementation option, removing the check for feature presence in selectedFeatures. This streamlines the logic and avoids redundant checks.
@jamesfredley
Copy link
Contributor Author

@matrei I updated the shouldApply() methods for the 3 servlet container features and retested. They are concise and understandable now.

@jamesfredley jamesfredley merged commit 18744d5 into 7.0.x Aug 22, 2025
30 checks passed
@jamesfredley jamesfredley deleted the forge-servlet-selection branch August 22, 2025 14:56
@github-project-automation github-project-automation bot moved this from In Progress to Done in Apache Grails Aug 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants