Skip to content

Fix Helm chart values.yaml having github-container-registry as default imagePullSecret#47

Merged
ThoSap merged 7 commits intomainfrom
fix-helm-chart-having-github-container-registry-as-image-pull-secret-default
Apr 1, 2026
Merged

Fix Helm chart values.yaml having github-container-registry as default imagePullSecret#47
ThoSap merged 7 commits intomainfrom
fix-helm-chart-having-github-container-registry-as-image-pull-secret-default

Conversation

@ThoSap
Copy link
Copy Markdown
Contributor

@ThoSap ThoSap commented Apr 1, 2026

No description provided.

@ThoSap ThoSap requested a review from stplasim April 1, 2026 07:56
@ThoSap ThoSap self-assigned this Apr 1, 2026
template:
spec:
affinity: {}
imagePullSecrets: [{}]
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Doesn’t this cause a Kubernetes validation issue? It renders the following in the Helm chart:

imagePullSecrets:
  - {}

Each imagePullSecrets entry is supposed to have a name property.

You could either:

  • Remove the field entirely if no secrets are needed.
  • Use an empty array as a placeholder

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I did not manage to fix the generated values.yaml file to not generate

app:
  imagePullSecrets:
    - {}

but I wrote a test that validates that this indeed works with the Kubernetes (at least with v1.34) API.

For K3s I can't select v1.35 yet.

In a follow-up PR I would like to add a CI test matrix that tests the operator against a range of still supported K8s versions.
It should build the image before the test matrix (without publishing it yet) and then install the Helm chart and validate that the deployment is up and running.

Copy link
Copy Markdown
Contributor Author

@ThoSap ThoSap left a comment

Choose a reason for hiding this comment

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

Just some comments to better understand these PR changes.

Comment on lines +136 to +150
ProcessBuilder.newBuilder(
"helm",
"install", releaseName, chartPath.toAbsolutePath().toString(), "--set", rootValuesAlias + ".image=postgresql-operator:test"
).environment(Map.of(
ENV_VAR_KUBECONFIG,
kubeConfigPath.toAbsolutePath().toString()
))
.exitCodeChecker(ec -> {
holder.exitCode = ec;
return true;
})
.error().redirect()
.output()
.consumeLinesWith(65536, line -> installOutput.append(line).append(System.lineSeparator()))
.run();
Copy link
Copy Markdown
Contributor Author

@ThoSap ThoSap Apr 1, 2026

Choose a reason for hiding this comment

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

I chose to use the io.smallrye.common.process.ProcessBuilder, which is used throughout the Quarkus codebase and makes starting the Helm commands easier in this test.

See the discussion quarkusio/quarkus#48223

See the actions (PRs where they applied this concept) in Quarkus:
quarkusio/quarkus#48223 (comment)

Comment on lines +69 to +94
// 1. Verify files exist and contain expected data
// ./Chart.yaml
@SuppressWarnings("unchecked")
Map<String, Object> chartMetadata = Serialization.yamlMapper()
.readValue(
chartPath.resolve("Chart.yaml").toFile(),
Map.class
);

assertThat(chartMetadata.get("name")).isEqualTo(chartName);

// ./values.yaml
@SuppressWarnings("unchecked")
Map<String, Object> values = Serialization.yamlMapper()
.readValue(
chartPath.resolve("values.yaml").toFile(),
Map.class
);

assertThat(values).containsKey(rootValuesAlias);

@SuppressWarnings("unchecked")
var appValues = (Map<String, Object>) values.get(rootValuesAlias);

Objects.requireNonNull(appValues, "appValues should not be null");
assertThat(appValues.get("image")).isNotNull();
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

jvmArgs("-javaagent:${mockitoAgent.absolutePath}")
val mockitoAgentProvider = configurations.named("testRuntimeClasspath").map { classpath ->
classpath.find { it.name.contains("mockito-core") }
}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Me

Why is it better to use this instead

val mockitoAgentProvider = configurations.named("testRuntimeClasspath").map { classpath ->
    classpath.find { it.name.contains("mockito-core") }
}

of the following?

val mockitoAgent = configurations.testRuntimeClasspath.get().find {
    it.name.contains("mockito-core")
}

Gemini 3.1 Pro

The difference comes down to one of the most important concepts in modern Gradle: Eager vs. Lazy Evaluation (Configuration Avoidance).

Here is why configurations.named("...").map { ... } is strictly better than configurations.testRuntimeClasspath.get().find { ... }:

1. The Configuration Phase vs. Execution Phase

Gradle builds happen in two main phases:

  1. Configuration Phase: Gradle reads all build.gradle.kts files and builds a graph of all tasks and dependencies.
  2. Execution Phase: Gradle actually runs the tasks (like compiling code or running tests).

2. What .get() does (Eager Evaluation)

When you use configurations.testRuntimeClasspath.get(), you are telling Gradle:
"Stop whatever you are doing and resolve all test dependencies right now, during the configuration phase."

The Problems:

  • Slows down the build: Dependency resolution involves reading POM files, checking local caches, and sometimes making network requests. Doing this during the configuration phase means your build will be slower every time you run any Gradle command (even if you just run gradlew tasks or gradlew clean).
  • Locks the configuration: Once a configuration is resolved via .get(), Gradle locks it. If another plugin or script later tries to add a dependency to testRuntimeClasspath, the build will crash with an error like: “Cannot change configuration 'testRuntimeClasspath' after it has been resolved.”

3. What .named().map() does (Lazy Evaluation)

When you use configurations.named("testRuntimeClasspath").map { ... }, you are creating a Provider.
You are telling Gradle:
"I will need this file eventually, but don't figure it out right now. Here are the instructions on how to find it when the time comes."

The Benefits:

  • Fast Configuration: Gradle just stores the instructions (the lambda) and moves on instantly. The configuration phase remains fast.
  • Resolved only when needed: The dependency resolution and the .find { ... } search only happen during the Execution Phase, and only if you actually run a task that needs it (like gradlew test). If you run gradlew build, and the test task is up-to-date or skipped, the resolution never happens at all.
  • Safe for other plugins: Because it's deferred, other scripts and plugins can safely continue modifying the testRuntimeClasspath configuration before the execution phase begins.

In summary, .get() forces Gradle to do heavy work immediately (which can break things and slow down your build), while .named().map() postpones the work until the exact moment it is actually required.

Comment on lines +85 to 98
class MockitoArgumentProvider(
@get:Optional
@get:InputFile
@get:PathSensitive(PathSensitivity.NONE)
val agentProvider: Provider<File>
) : CommandLineArgumentProvider {
override fun asArguments(): Iterable<String> {
val agentFile = agentProvider.orNull
return if (agentFile != null) {
listOf("-javaagent:${agentFile.absolutePath}")
} else {
emptyList()
}
}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We have to do this, else we get

FAILURE: Build failed with an exception.
* What went wrong:
Configuration cache problems found in this build.
1 problem was found storing the configuration cache.
- Task `:operator:test` of type `org.gradle.api.tasks.testing.Test`: cannot serialize Gradle script object references as these are not supported with the configuration cache.
  See https://docs.gradle.org/9.4.1/userguide/configuration_cache_requirements.html#config_cache:requirements:disallowed_types

Gemini 3.1 Pro

The error cannot serialize Gradle script object references occurs because the Configuration Cache needs to serialize the task state to disk, but the CommandLineArgumentProvider lambda implicitly captures a reference to the whole build.gradle.kts script object (which cannot be serialized).

To make this fully compatible with the Configuration Cache, you need to use a distinct, explicitly defined class for the CommandLineArgumentProvider that explicitly declares its inputs, rather than using a Kotlin lambda.

Here is the correct Configuration Cache compatible solution:

// 1. Define a concrete class for the argument provider.
// This prevents capturing the Gradle script implicitly and tells Gradle exactly what the inputs are.
class MockitoArgumentProvider(
    @get:Optional
    @get:InputFile
    @get:PathSensitive(PathSensitivity.NONE)
    val agentProvider: Provider<File>
) : CommandLineArgumentProvider {
    override fun asArguments(): Iterable<String> {
        val agentFile = agentProvider.orNull
        return if (agentFile != null) {
            listOf("-javaagent:${agentFile.absolutePath}")
        } else {
            emptyList()
        }
    }
}

// 2. Resolve the provider lazily
val mockitoAgentProvider = configurations.named("testRuntimeClasspath").map { classpath ->
    classpath.find { it.name.contains("mockito-core") }
}

tasks.withType<Test>().configureEach {
    dependsOn(tasks.quarkusAppPartsBuild)

    // 3. Add the provider class instance instead of a lambda
    jvmArgumentProviders.add(MockitoArgumentProvider(mockitoAgentProvider))
}

Why this fixes the issue:

  1. No Implicit Capturing: By defining a separate class, we avoid Kotlin's lambda capture mechanics which inadvertently drag the entire build script instance into the configuration cache.
  2. Proper Input Tracking: By annotating agentProvider with @InputFile and @Optional, we tell Gradle exactly what file this argument provider depends on. Gradle can now properly track this file for Task caching, and it natively knows how to serialize Provider<File> types to the Configuration Cache.

@ThoSap ThoSap requested a review from stplasim April 1, 2026 12:36
@ThoSap ThoSap merged commit 7de8101 into main Apr 1, 2026
4 checks passed
@ThoSap ThoSap deleted the fix-helm-chart-having-github-container-registry-as-image-pull-secret-default branch April 1, 2026 13:50
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