SLING-12283 : changed the config Pid to separate factoryPID & PID wit… #13
Conversation
There was a problem hiding this comment.
Hi @rishabhdaim and thanks for the PR. I am still running some tests regarding your code changes, hopefully done today or tomorrow.
I left some comments in the meantime.
src/main/java/org/apache/sling/installer/factories/configuration/impl/ConfigUtil.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/sling/installer/factories/configuration/impl/ConfigUtil.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/sling/installer/factories/configuration/impl/ConfigTaskCreator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/sling/installer/factories/configuration/impl/ConfigTaskCreator.java
Outdated
Show resolved
Hide resolved
|
@rishabhdaim - can you please merge with the latest master commit? fd459da would make our testing easier. |
… exiting with legacy name or not
|
| if (config == null) { | ||
| config = ConfigUtil.getLegacyFactoryConfig(this.getConfigurationAdmin(), this.factoryPid, null, this.configPid); | ||
| } |
There was a problem hiding this comment.
I don't think this is a complete fix (together with apache/sling-org-apache-sling-installer-provider-jcr#9). The original.pid property is not leveraged. Installation would need to be skipped if the original.pid property is present AND a configuration with that PID exists in config admin.
There was a problem hiding this comment.
I agree that I haven't checked whether the original.pid property is present or not cause I didn't need it (may be we can remove it altogether).
Regarding the checking of config, the ConfigUtil.getLegacyFactoryConfig does check it here
There was a problem hiding this comment.
I think this change here is actually "good enough".
…h ~ before saving it