SLING-11497 - convert all cnd files to repoinit statements #143
Conversation
Make sure to include all cnd files to the aclmanager in sling initial content. This will result in having the cnd definitions be converted to repoinit statements.
| @@ -138,6 +141,11 @@ private void extractFile(JarEntry jarEntry, JarOutputStream bundleOutput) throws | |||
|
|
|||
| SlingInitialContentBundleEntryMetaData bundleEntry = createSlingInitialContentBundleEntry(context, targetFile); | |||
| collectedSlingInitialContentBundleEntries.add(bundleEntry); | |||
| } else if (jarEntry.getName().endsWith(".cnd")) { | |||
There was a problem hiding this comment.
IMHO only those cnd files referenced in the manifest are considered in Sling Initial Content (https://sling.apache.org/documentation/bundles/content-loading-jcr-contentloader.html#node-type-and-namespace-definitions-cnd)
There was a problem hiding this comment.
Good point. Let me work on that.
There was a problem hiding this comment.
@kwin implemented & pushed
Narrow down the nodetype registration to Sling-Nodetypes header
| return false; | ||
| } | ||
|
|
||
| return jarEntry.getName().equals(slingNodeTypes); |
There was a problem hiding this comment.
This is potentially a comma separated list. Also a unit test/IT covering this would be good.
There was a problem hiding this comment.
Again a good point.. implemented this and pushed.
| @@ -109,12 +109,6 @@ void extractAndAddToAssembler(@NotNull BundleSlingInitialContentExtractContext c | |||
| throw new IOException("Can not convert " + file + " to enhanced DocView format", e); | |||
| } | |||
|
|
|||
| // remap CND files to make sure they are picked up by the NodeTypesEntryHandler | |||
| if (context.getNamespaceRegistry().getRegisteredCndSystemIds().contains(file.getName())) { | |||
| contentPackageEntryPath = "/META-INF/vault/" + Text.getName(file.getName()) + ".cnd"; | |||
There was a problem hiding this comment.
Why was the approach to convert to cnd files in a package discarded?
There was a problem hiding this comment.
This doesn't seem to have an effect afaik but I guess im wrong, il put it back in then
There was a problem hiding this comment.
Either put into repo init statements or into content packages, not both!
Make Sling-Nodetypes multi valued, separated by comma Unit test added + verify
Add integration test
|
Kudos, SonarCloud Quality Gate passed! |
|
@kwin - do you have other comments regarding this PR? |
|
For me the general approach of converting CNDs is not clear. In the past they were supposed to be wrapped in content-packages (coming from Sling Initial Content) and now they are supposed to be converted to RepoInit. Not sure if there former way was fully removed by this PR. Also the same conversion should be done for CND files coming from regular content packages (not via Sling-Initial Content). For that I don't see a test either. |
|
Thanks @kwin , I guess we need @niekraaijmakers to look into this in more depth then. |
|
@kwin so my question is, on the current state (master) where is the namespace registration handled? Wrapped in a content package? Because when I execute it in a unit test ( the one in this PR ) I can't find it, but I might just be missing it. Should the CND file remain in the core package itself? |
I don't know, my point is rather that this should be handled the same no matter whether the CND is provided through a package or via initial content. Please proof with additional test that Also it would be good to add the information to the readme how namespace and node type registration is handled. |
Make sure to include all cnd files to the aclmanager in sling initial content.
This will result in having the cnd definitions be converted to repoinit statements.