Skip to content

Conversation

@C0urante
Copy link
Contributor

@C0urante C0urante commented Sep 9, 2022

Follow-up from #12609. Non-urgent, just tidies up the build file a bit.

Tested and verified locally by:

  1. Running ./gradlew releaseTarGz and then examining the resulting release artifact with tar tf tar tf core/build/distributions/kafka_2.13-3.4.0-SNAPSHOT.tgz to ensure that it did not contain the swaggerJaxrs2 jar or its dependencies
  2. Running ./gradlew siteDocsTar and then examining the resulting docs with cat docs/generated/connect_rest.yaml to ensure that all Connect REST endpoints were included.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@C0urante C0urante force-pushed the kafka-14198-separate-swagger-configuration branch from 9091739 to 09f1f0b Compare September 9, 2022 15:00
@@ -2564,7 +2568,9 @@ project(':connect:runtime') {
implementation libs.mavenArtifact
implementation libs.swaggerAnnotations
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this to be implementation or can it be swagger too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I gave this a try but it wasn't successful. We still need the annotations at compile time since they're used to decorate our REST resource classes, like here:

@GET
@Path("/")
@Operation(summary = "List all active connectors")
public Response listConnectors(
final @Context UriInfo uriInfo,
final @Context HttpHeaders headers
) {


task genConnectOpenAPIDocs(type: io.swagger.v3.plugins.gradle.tasks.ResolveTask, dependsOn: setVersionInOpenAPISpec) {
classpath = sourceSets.main.compileClasspath + sourceSets.main.runtimeClasspath
classpath = sourceSets.main.runtimeClasspath
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this assignment at all or can we just inline it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The plugin requires the classpath field to be set.

I also tried this:

classpath = sourceSets.main.runtimeClasspath + configurations.swagger.asCollection()

buildClasspath = classpath

which led to a lot of these warnings:

Gradle detected a problem with the following location... Reason: Task ':connect:runtime:genConnectOpenAPIDocs' uses this output of task ':connect:runtime:processResources' without declaring an explicit or implicit dependency.

Based on discussion in the PR that introduced OpenAPI docs generation for Connect, it appears that setting classpath to sourceSets.main.runtimeClasspath creates some implicit dependencies for the genConnectOpenAPIDocs task that fix these warnings.

@ijuma
Copy link
Member

ijuma commented Sep 9, 2022

Thanks @C0urante. Did you verify that the documentation is still generated correctly and the release tarball still doesn't have swagger and snakyaml in the dependencies?

@C0urante C0urante changed the title KAFKA-14198: Define separate swagger configuration KAFKA-14198: Define separate swagger configuration in build file Sep 12, 2022
@C0urante C0urante changed the title KAFKA-14198: Define separate swagger configuration in build file KAFKA-14198: Define separate swagger configuration in Gradle build Sep 12, 2022
@C0urante
Copy link
Contributor Author

Thanks @ijuma. Yes, I verified both, and updated the PR description with the steps I took to do so.

Copy link
Member

@ijuma ijuma left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@C0urante
Copy link
Contributor Author

Thanks Ismael. Kicked off another Jenkins build since the previous one failed with a disk space error; will merge if everything looks alright.

@C0urante
Copy link
Contributor Author

Failures appear unrelated, merging.

@C0urante C0urante merged commit e43e59f into apache:trunk Sep 12, 2022
@C0urante C0urante deleted the kafka-14198-separate-swagger-configuration branch September 12, 2022 16:33
@C0urante
Copy link
Contributor Author

@ijuma @jsancio I'm assuming we don't need to backport this to 3.3 since the immediate issues with Swagger deps and Connect OpenAPI docs generation have already been fixed on that branch; let me know if that's not the case, though.

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.

2 participants