Re: [PR] SLING-12417- [Compiled Scripts] Add healthcheck for loading capabilities [sling-org-apache-sling-servlets-resolver]

[via GitHub]

joerghoh merged PR #50:
URL: https://github.com/apache/sling-org-apache-sling-servlets-resolver/pull/50


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@sling.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] SLING-12417- [Compiled Scripts] Add healthcheck for loading capabilities [sling-org-apache-sling-servlets-resolver]

[via GitHub]

joerghoh commented on code in PR #50:
URL: 
https://github.com/apache/sling-org-apache-sling-servlets-resolver/pull/50#discussion_r1860704391


##
src/main/java/org/apache/sling/servlets/resolver/internal/bundle/BundledScriptTracker.java:
##
@@ -136,10 +154,23 @@ protected void deactivate() {
 if (bt != null) {
 bt.close();
 }
+if (healthCheckRegistration != null) {
+healthCheckRegistration.unregister();
+healthCheckRegistration = null;
+}
 bundleContext.set(null);
 dispatchers.set(null);
 }
 
+@SuppressWarnings({ "rawtypes", "unchecked" })
+ServiceRegistration registerHealthCheck(String[] tags) {
+Dictionary props = new Hashtable();
+props.put(HealthCheck.NAME, "Sightly BundledScriptTracker 
Healthcheck");

Review Comment:
   removed the "Sightly" from the property name.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@sling.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] SLING-12417- [Compiled Scripts] Add healthcheck for loading capabilities [sling-org-apache-sling-servlets-resolver]

[via GitHub]

joerghoh commented on PR #50:
URL: 
https://github.com/apache/sling-org-apache-sling-servlets-resolver/pull/50#issuecomment-2498334469

   @raducotescu ok, got it.
   
   Assuming your case, wouldn't it be reasonable to assume that version 2.0 and 
version 1.0 have a large overlap of bundled scripts? That means: I don't know 
how the resolver would behave when both bundles would register a script for 
``foo/bar/script``. In my opinion the result is unpredictable.
   
   And for that I would suggest that we don't complicate this healthcheck until 
we support this.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@sling.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] SLING-12417- [Compiled Scripts] Add healthcheck for loading capabilities [sling-org-apache-sling-servlets-resolver]

[via GitHub]

raducotescu commented on code in PR #50:
URL: 
https://github.com/apache/sling-org-apache-sling-servlets-resolver/pull/50#discussion_r1856586287


##
src/main/java/org/apache/sling/servlets/resolver/internal/bundle/BundledScriptTracker.java:
##
@@ -756,4 +805,16 @@ private Set 
reduce(Set
 newSet.addAll(originalCapabilities);
 return newSet;
 }
+
+
+@ObjectClassDefinition
+public @interface BundledScriptTrackerConfig {
+
+@AttributeDefinition(name="Mandatory Bundles", description="A list of 
symbolic bundle names the for which the "
++ "script registration process must have been successfully 
completed for the health check to report ok.")
+String[] mandatoryBundles();

Review Comment:
   ```suggestion
   @AttributeDefinition(name="Mandatory Bundles", description="A list 
of symbolic bundle names for which the "
   + "script registration process must have been successfully 
completed for the health check to report ok.")
   String[] mandatoryBundles();
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@sling.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] SLING-12417- [Compiled Scripts] Add healthcheck for loading capabilities [sling-org-apache-sling-servlets-resolver]

[via GitHub]

raducotescu commented on PR #50:
URL: 
https://github.com/apache/sling-org-apache-sling-servlets-resolver/pull/50#issuecomment-2497983321

   > @raducotescu wrote:
   >> would it make sense to add support for multiple versions of the same 
bundle
   
   > I don't know, what you mean with that in this context of this PR. Can you 
please elaborate?
   
   Right now when we compare the expected to the registered bundles we only 
look at bundle symbolic names. Assuming we'd have two bundles B with different 
versions, say 1.0 and 2.0, and a configuration for an expected bundle B, we'd 
return ok from the health check if either of the two actual bundles B has 
registered its scripts.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@sling.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] SLING-12417- [Compiled Scripts] Add healthcheck for loading capabilities [sling-org-apache-sling-servlets-resolver]

[via GitHub]

joerghoh commented on PR #50:
URL: 
https://github.com/apache/sling-org-apache-sling-servlets-resolver/pull/50#issuecomment-2494118002

   @raducotescu wrote:
   
   > would it make sense to add support for multiple versions of the same bundle
   
   I don't know, what you mean with that in this context of this PR. Can you 
please elaborate?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@sling.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] SLING-12417- [Compiled Scripts] Add healthcheck for loading capabilities [sling-org-apache-sling-servlets-resolver]

[via GitHub]

raducotescu commented on code in PR #50:
URL: 
https://github.com/apache/sling-org-apache-sling-servlets-resolver/pull/50#discussion_r1853000481


##
src/main/java/org/apache/sling/servlets/resolver/internal/bundle/BundledScriptTracker.java:
##
@@ -557,6 +589,23 @@ public void removedBundle(Bundle bundle, BundleEvent 
event, List 
reduce(Set
 newSet.addAll(originalCapabilities);
 return newSet;
 }
+
+
+@ObjectClassDefinition
+public @interface BundledScriptTrackerConfig {
+
+@AttributeDefinition(name="Mandatory Bundles", description="for all of 
the provided symbolic bundle names the "
++ "registration process must have been completed successfully 
for the healthcheck to report ok")
+String[] mandatoryBundles();

Review Comment:
   ```suggestion
   @AttributeDefinition(name="Mandatory Bundles", description="A list 
of symbolic bundle names the for which the "
   + "script registration process must have been successfully 
completed for the health check to report ok.")
   String[] mandatoryBundles();
   ```



##
src/main/java/org/apache/sling/servlets/resolver/internal/bundle/BundledScriptTracker.java:
##
@@ -136,10 +154,23 @@ protected void deactivate() {
 if (bt != null) {
 bt.close();
 }
+if (healthCheckRegistration != null) {
+healthCheckRegistration.unregister();
+healthCheckRegistration = null;
+}
 bundleContext.set(null);
 dispatchers.set(null);
 }
 
+@SuppressWarnings({ "rawtypes", "unchecked" })
+ServiceRegistration registerHealthCheck(String[] tags) {
+Dictionary props = new Hashtable();
+props.put(HealthCheck.NAME, "Sightly BundledScriptTracker 
Healthcheck");

Review Comment:
   This health check doesn't have anything to do with Sightly/HTL.



##
src/main/java/org/apache/sling/servlets/resolver/internal/bundle/BundledScriptTracker.java:
##
@@ -557,6 +589,23 @@ public void removedBundle(Bundle bundle, BundleEvent 
event, List