Skip to content

Conversation

@adutra
Copy link
Contributor

@adutra adutra commented Nov 3, 2025

This PR automates Helm tests and make them executable as part of the Gradle build.

This PR automates Helm tests and make them executable as part of the Gradle build.
dimas-b
dimas-b previously approved these changes Nov 3, 2025
Copy link
Contributor

@dimas-b dimas-b left a comment

Choose a reason for hiding this comment

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

LGTM 👍

CONTRIBUTING.md Outdated
* [SDKMAN!](https://sdkman.io/) - Run `sdk list java` to see available distributions, then `sdk install java <identifier>` to install.
* [jEnv](https://www.jenv.be/) - You can also use jEnv to manage Java versions.

* **Docker**: Required for integration tests and building container images.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Docker or Podman? Podman with its docker compatibility shims works fairly well in my local build.

@github-project-automation github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Nov 3, 2025
@dimas-b dimas-b requested a review from snazy November 3, 2025 17:08
Copy link
Member

@snazy snazy left a comment

Choose a reason for hiding this comment

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

This is nice!

Some comments - all Gradle related improvements.

description = "Apache Polaris (incubating) Helm Chart"

val helmTestReportsDir = layout.buildDirectory.dir("reports")

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
val runtimeServerDistribution by configurations.registering {
isCanBeConsumed = true
isCanBeResolved = false
}
dependencies {
runtimeServerDistribution(project(":polaris-server", "distributionElements"))
}

workingDir = rootProject.projectDir

// Output: file containing SHA digest of built image
val outputFile = helmTestReportsDir.get().file("minikube-images.sha256")
Copy link
Member

Choose a reason for hiding this comment

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

Better relativize outputFile to project.projectDir (or better this.workingDir) here (and in other places) so the commandLine doesn't contain any local paths.

Within a task configuration block, you can

Suggested change
val outputFile = helmTestReportsDir.get().file("minikube-images.sha256")
val outputFile = helmTestReportsDir.get().file("minikube-images.sha256").relativeTo(project.projectDir)


dependsOn(":polaris-server:quarkusBuild")

inputs.dir(rootProject.file("runtime/server/build/quarkus-app"))
Copy link
Member

Choose a reason for hiding this comment

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

Use the approach from above.

group = "documentation"
description = "Generate Helm chart documentation using helm-docs"

workingDir = rootProject.projectDir
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
workingDir = rootProject.projectDir

Comment on lines 242 to 245
tasks.named("build") {
dependsOn(intTest)
dependsOn(helmDocs)
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
tasks.named("build") {
dependsOn(intTest)
dependsOn(helmDocs)
}
tasks.named("assemble") { dependsOn(helmDocs) }

Comment on lines +148 to +155
make install-dependencies-brew
make install-optional-dependencies-brew
Copy link
Member

Choose a reason for hiding this comment

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

Mind adding this as a macOS variant but leave the Linux one?

(Also for the other occurrences below)

dependsOn(chartTestingInstall)
}

tasks.named("check") { dependsOn(test) }
Copy link
Member

Choose a reason for hiding this comment

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

check is expected to exercise checks

Suggested change
tasks.named("check") { dependsOn(test) }
tasks.named("check") { dependsOn(test, intTest) }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So check is expected to run intTest? TIL :-)

./gradlew \
intTest \
-x :polaris-runtime-service:intTest \
-x :polaris-helm:intTest \
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
-x :polaris-helm:intTest \
-x :polaris-helm:check \

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So check doesn't work, :polaris-helm:intTest still gets executed.


dependsOn(":polaris-server:quarkusBuild")

inputs.files(runtimeServerDistribution)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@snazy I think this is the only task that actually needs this configuration as input.

As for the Dockerfile, I don't see any other way to reference it as it is not an output of any task (that I know of). Should I create a configuration in polaris-server to expose it?

@adutra adutra force-pushed the gradle-helm branch 2 times, most recently from 9070130 to 1e4db2c Compare November 5, 2025 14:10
@adutra
Copy link
Contributor Author

adutra commented Nov 5, 2025

To all the reviewers: sorry for so pushing so many commits 😅 – it was harder than I thought to get this PR right.

Note: with this change, ./gradlew check or ./gradlew build would fail if the necessary tools are not installed (helm, ct, minikube...). I think that's fair but happy to discuss this if someone disagrees.

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.

4 participants