-
Notifications
You must be signed in to change notification settings - Fork 229
Merge Grails Spring Security Plugin repositories #1057
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
Conversation
WIP - Publishing
…s-jose-jwt-9.x Update dependency com.nimbusds:nimbus-jose-jwt to v9.41.2
Update pac4jVersion to v5.7.7
Bumps [commons-codec:commons-codec](https://github.com/apache/commons-codec) from 1.16.0 to 1.16.1. - [Changelog](https://github.com/apache/commons-codec/blob/master/RELEASE-NOTES.txt) - [Commits](apache/commons-codec@rel/commons-codec-1.16.0...rel/commons-codec-1.16.1) --- updated-dependencies: - dependency-name: commons-codec:commons-codec dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [commons-codec:commons-codec](https://github.com/apache/commons-codec) from 1.16.0 to 1.16.1. - [Changelog](https://github.com/apache/commons-codec/blob/master/RELEASE-NOTES.txt) - [Commits](apache/commons-codec@rel/commons-codec-1.16.0...rel/commons-codec-1.16.1) --- updated-dependencies: - dependency-name: commons-codec:commons-codec dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [net.bytebuddy:byte-buddy](https://github.com/raphw/byte-buddy) from 1.14.11 to 1.14.19. - [Release notes](https://github.com/raphw/byte-buddy/releases) - [Changelog](https://github.com/raphw/byte-buddy/blob/master/release-notes.md) - [Commits](raphw/byte-buddy@byte-buddy-1.14.11...byte-buddy-1.14.19) --- updated-dependencies: - dependency-name: net.bytebuddy:byte-buddy dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com>
…mons-codec-commons-codec-1.16.1 Bump commons-codec:commons-codec from 1.16.0 to 1.16.1
Update dependency com.google.guava:guava to v33.3.1-jre
Update dependency gradle to v7.6.4
…mons-codec-commons-codec-1.16.1 Bump commons-codec:commons-codec from 1.16.0 to 1.16.1
….bytebuddy-byte-buddy-1.14.19 Bump net.bytebuddy:byte-buddy from 1.14.11 to 1.14.19
…ons-codec-1.x Update dependency commons-codec:commons-codec to v1.17.1
Disable `standardOut` and `standardError` test event output
Because GitHub action stalled.
Removed PDF and EPUB from `acl` and `cas` for now.
jdaugherty
left a comment
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.
My main concerns are:
- the doc publishing, i think only core is publishing
- the nested readme files need updated to remove their badges and links to refer to the new repo
I pointed out some out dated code in the docs, but we can ticket to fix that later if you want.
If I can help with the doc publishing, please let me know. I'm happy to make those changes to expedite this.
plugin-cas/LICENSE
Outdated
| @@ -0,0 +1,201 @@ | |||
| Apache License | |||
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.
Only the license file at the root of the project needs to remain; this can be removed.
plugin-ldap/LICENSE
Outdated
| @@ -0,0 +1,201 @@ | |||
| Apache License | |||
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.
Only the license file at the root of the project needs to remain; this can be removed.
plugin-oauth2/LICENSE
Outdated
| @@ -0,0 +1,201 @@ | |||
| Apache License | |||
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.
Only the license file at the root of the project needs to remain; this can be removed.
plugin-rest/LICENSE
Outdated
| @@ -0,0 +1,201 @@ | |||
| Apache License | |||
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.
Only the license file at the root of the project needs to remain; this can be removed.
gradle.properties
Outdated
| commonsLangVersion=2.6 | ||
| dumbsterVersion=1.6 | ||
| grailsRedisVersion=5.0.0-M1 | ||
| hibernateVersion=5.6.15.Final |
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.
I see this is being used under acl-examples-functional-test-app, but the gorm-hibernate5 should already be including this library. Why are we including it separately and having to maintain the hibernate version separately?
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.
acl-examples-functional-test-appis usingorg.hibernate.ObjectNotFoundExceptionui-examples-extendedandui-examples-simpleare usingorg.hibernate.SessionFactory
This makes org.hibernate:hibernate-core-jakarta a direct dependency of these projects, so it should be declared as such. If we add the version to grails-bom (hibernate5.version?) or when we move to Hibernate 6 (hibernate.version is available in spring-boot-dependencies) we can omit this.
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.
What are your thoughts on exposing hibernate5 as an API as part of gorm-hibernate5?
ObjectNotFoundException is directly thrown by DomainClass.get(id) - if you want performant applications, you'll have to add @GrailsCompileStatic which means any place someone uses a get() method, they will be forced to add this gradle library. I suspect that means pretty much everywhere for any real world use of hibernate.
Again, if type checking is enabled, the Hibernate Session class will be the argument to helpers like withNewSession { }.
I'd normally agree you have to manually expose these, but with the hibernate5 not being able to be put in the bom, the gorm implementation acting as a wrapper around hibernate, and the compatibility issues between 5 & 6, it seems worth to expose as an API dependency.
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.
Wait, looking closer at the gorm-hibernate5 project:
api "org.hibernate:hibernate-core-jakarta:$hibernateVersion", {
It's already added as an api. We shouldn't be overriding it since that's a maintenance burden.
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.
Yes, it adds a minimal maintenance burden (in this case I think the burden is actually zero as I don't think the version will change), but declaring your direct dependencies is a best practice that also brings stability and clarity.
If you still oppose this I will remove the dependency declaration, as these are only example/test apps, but for libraries I think we should be more rigid as not adhering to this has been a source problems and confusion historically (at least for me).
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.
I 100% agree we should be declaring direct dependencies for our libraries / internal projects to grails-core. We should be as detailed as possible in our gradle projects given the scope.
With that said, I want to be careful about having end grails apps do so. These are examples, so they are end apps in my mind. I disagree with adding extra dependencies here. Developers are going to use this as an example and I view gorm-hibernate5 as an extension of hibernate5, not an addition. We want our examples to represent end user apps and we don't want the user to have to redefine these. There's the added benefit that it's less maintenance burden long term as we upgrade too.
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.
Not sure why there's a nested project under the docs, can this whole project be moved out to a test example and referenced? I'm going to stop making comments similar to this on everything in the code directory under docs
.github/workflows/gradle.yml
Outdated
| MAVEN_PUBLISH_PASSWORD: ${{ secrets.MAVEN_PUBLISH_PASSWORD }} | ||
| run: ./gradlew --no-build-cache publish | ||
| - name: "🔨 Generate Snapshot Documentation" | ||
| run: ./gradlew core-docs:docs |
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.
This only publishes the core-docs. Do we intend to combine the docs together or leave separate and publish as subdirectories? My first reaction is to keep them separate.
plugin-oauth2/README.md
Outdated
| @@ -0,0 +1,78 @@ | |||
| Spring Security OAuth2 Plugin | |||
| ======= | |||
| [](https://github.com/grails/grails-spring-security-oauth2/actions/workflows/gradle.yml) | |||
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.
Can we remove the badges from this file since they won't be valid anymore?
| ```groovy | ||
| dependencies { | ||
| ... | ||
| implementation 'org.grails.plugins:spring-security-core:5.2.1' |
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.
Can we move the installation instructions to the docs so the versions can be updated as part of the build process?
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.
I'll defer updating the README files to another PR.
plugin-rest/docs/build.gradle
Outdated
| 'icons' : 'font', | ||
| 'version' : project.version, | ||
| 'sourcedir' : "${rootProject.allprojects.find { it.name == 'spring-security-rest' }.projectDir}/src/main/groovy", | ||
| 'baseGroovyApiUrl': "https://grails-plugins.github.io/grails-spring-security-rest/${project.version}/docs/gapi/" |
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.
I believe this link will be invalid now.
Only keep the LICENSE file in the root project directory.
These files are not used anymore as the projects are merged and only need one `ghpages.html` file.
For removing aggregated docs.
Plus some other housekeeping.
plugin-cas/docs/src/docs/index.adoc
Outdated
| @@ -1,15 +1,14 @@ | |||
| :includedir: src/docs/ | |||
| = Spring Security CAS Plugin - Reference Documentation | |||
| Puneet Behl <behlp@unityfoundation.io> | |||
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.
Should we remove this since we dont' typically put the original author or maintainers in the docs at the top?
This PR merges the following Grails Spring Security Plugins into this repository: