-
Notifications
You must be signed in to change notification settings - Fork 229
Revert to Sitemesh 2 #1144
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
Revert to Sitemesh 2 #1144
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
The dependencies in the example projects still need cleaned up - so they only include coordinates that we expect users to include (org.apache.grails). Otherwise, we'll have to reroll this again in the next release.
-
Can we add the debug in test-config.gradle that I added in the other PR so that we can comment in the future to help troubleshoot?
-
Can we turn off the spring dependency management plugin for the non-examples / plugins?
| implementation 'org.apache.grails:grails-core' | ||
| implementation 'org.apache.grails:grails-gsp' | ||
| implementation "org.apache.grails.views:grails-layout:$grailsVersion" | ||
| implementation 'org.apache.grails.views:grails-layout' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't be including internal libraries - in the M5 it's supposed to be included via the include of GSP. Including this here will cause it to break again on the RC1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See: #1144 (comment)
| implementation 'org.apache.grails:grails-converters' | ||
| implementation 'org.apache.grails:grails-gsp' | ||
| implementation "org.apache.grails.views:grails-layout:$grailsVersion" | ||
| implementation 'org.apache.grails.views:grails-layout' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't be including internal libraries - in the M5 it's supposed to be included via the include of GSP.Including this here will cause it to break again on the RC1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, it seems this has changed again after M5: apache/grails-core@cdbb4df?
So either way, grails-layout needs to be added back (in the org.apache.grails group) for the next release, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are correct, for the next release we should have to add it, but I would assume we would add it by include grails-dependencies-all instead. If you want to wait to do this change, I'm ok waiting until the next release.
|
I'm assuming the real fix here was the remove of the waitFors - and leaving the selenium workaround in place? |
No, I think it was removing the |
If you add |
OK, sorry for being unclear. I meant, I don't know how the titles could be output when you ran the tests (with reporting). |
|
I think we also need to change the ldap configuration for these projects like I did here: https://github.com/apache/grails-spring-security/pull/1143/files We're pointing to an external website to use ldap, instead of using one local for our test run. |
I don't understand why this is the case with functional test applications.
Are you referring to the Geb reporting?
Is the former still used?
I think this is already done in |
|
On the geb configuration, I was talking about this code: For the functional tests, we should be testing with the same dependencies that end applications will use. From what I can tell, there has been divergence from a generated application and the applications in this project. For the plugin-config.gradle, that's only applied to the plugins. I think we should turn the Spring Dependency Management gradle plugin off in the functional test apps because it can cause us to unknowingly pull from a repo instead of use the local projects. This happened multiple times in the grails-core merge, and the only reason we caught it was because we were changing coordinates. Using gradle platforms guarantees we pull dependencies from the local project and never from a configure repo. |
|
The LDAP configuration that I was speaking about are the changes to: plugin-ldap/examples/retrieve-db-roles/grails-app/conf/application.groovy Right now we're putting load on an external service (ldap.forumsys.com:389) and I've seen it block us. Our tests should use local ldap instead of relying on an external website. |
I have updated the |
When there is only one property to check, we do not need to iterate over a list.
- To prevent unexpected versions being pulled in - Also use type-safe API when configuring GrailsExtension
|
My only remaining concern here is the out of date dependencies on the functional test apps |
The thing is, we can't just update the versions. The CSS class names and HTML structure has changed in later versions of Bootstrap. If we set out to keep the UI of old functional test applications up-to-date, we will have a lot of work. I'm not saying it shouldn't ever be done, but I think it is out of scope for this PR. |
Grails 7.0.0-M5 has reverted to Sitemesh 2.