Skip to content

Conversation

@vnandwana
Copy link

  • Replaced javax.servlet with Jakarta servlet.

* Replaced javax.servlet with Jakarta servlet.
* Replaced javax.servlet with Jakarta servlet.
<dependency>
<groupId>org.jooq</groupId>
<artifactId>jooq</artifactId>
<!-- 3.14.0 broke returning() on MySQL :( -->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you confirm this is now fixed, by running the integration tests?

export STRIPE_API_KEY=...
export STRIPE_PUBLIC_KEY=...
mvn test -Pintegration-mysql

This has been a long standing bug in jooq that has prevented us from upgrading. It would be very good news if it's now addressed, many plugins could be upgraded.

<Match>
<Class name="~org\.killbill\.billing\.plugin\.stripe\.dao\.gen.*"/>
</Match>
<Bug pattern="EI_EXPOSE_REP"/>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignoring EI_EXPOSE_REP and EI_EXPOSE_REP2 is probably fine, however NP_PARAMETER_MUST_BE_NONNULL_BUT_MARKED_AS_NULLABLE seems a bit suspicious. Can you investigate what's triggering this?

}

try {
metricRegistry.getMetricRegistry().counter("stripe_plugin_counter").inc(1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code snippet from the hello world plugin is really just an example. We probably don't want it in plugins that are used in production code 😉

You could add something more useful here for instance:

return intent.capture(paymentIntentParams, requestOptions);

try {
  return intent.capture(paymentIntentParams, requestOptions);
} finally {
  metricRegistry.getMetricRegistry().counter("stripe_capture").inc(1);
}

Same for other Stripe client API calls.

final StripeHealthcheck stripeHealthcheck = new StripeHealthcheck(stripeConfigPropertiesConfigurationHandler);
registerHealthcheck(context, stripeHealthcheck);

// Expose metrics (optional)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not optional anymore 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants