Support KubernetesServer test resource for CRUD operations

[FroMage]

Can you check @shawkins if this would work for you?

It's a bit over-engineered, but I didn't want for future fixes on one class to miss the other.
I'm also unsure about the annotation for injecting the crud server, but the target server type doesn't have a good name including mock like the other… I don't you don't need it ATM but it's more regular to support injection anyway.

@geoand could you double-check if this is binary-compatible? I think it should be. Do you know if we have tests/docs?

Fixes #14744.

Late edit: Also fixes #14823.

[ghost]

Thanks for your pull request!

The title of your pull request does not follow our editorial rules. Could you have a look?

• title should not contain an issue number (use 'Fix Allow * expression for quarkus:add-extension #1234' in the description instead)

This message is automatically generated by a bot.

[geoand]

Sure thing, I'll check it later on today

[geoand]

Looks great to me!

I assume we also want to wait for feedback from @shawkins

[shawkins]

This looks good to me as well. I should add there is an implicit assumption that the KubernetesServer will always be in crud mode because if you didn't want that you would just use the non-crud test resource. However there is case to be made for something like the Junit EnableKubernetesMockClient where you can toggle the crud mode and use this new test resource either way.

[FroMage]

OK hold on don't merge, let me investigate what @shawkins said and another version of this feature I've spotted in the wild.

[geoand]

I'll let you merge it when you like

[FroMage]

OK I have added a test locally, but I have questions.

@geoand ATM SSL usage is defined by:

    protected boolean useHttps() {
        return Boolean.getBoolean("quarkus.kubernetes-client.test.https");
    }

Which is a Java system property. Shouldn't we obtain the quarkus config for quarkus.kubernetes-client.master-url instead? As in, would it be better for test users? And is it possible to use quarkus config at this lifecycle stage?

@kornys @shawkins I see in UseKubeMockServer that you want to configure ssl, crud and port.

I'm thinking of making those configurable using the quarkus config (see above), would it be OK for you or you prefer annotation attributes?

As for configuring crud, I can add an annotation attribute, but if it's false then why not just use the KubernetesMockServer?

I guess hard-coding it to crud=true would allow me to rename this KubernetesCrudServer. WDYT?

[geoand]

Which is a Java system property. Shouldn't we obtain the quarkus config for quarkus.kubernetes-client.master-url instead?

Yeah, that makes more sense.

As in, would it be better for test users? And is it possible to use quarkus config at this lifecycle stage?

Unfortunately that won't work because this method is run before any Quarkus bootstrapping has taken place (since one of the points of start is to affect the environment the application will run in)

[shawkins]

As for configuring crud, I can add an annotation attribute, but if it's false then why not just use the KubernetesMockServer?

I am saying the same thing above, that it's an implicit assumption of hardcoding crud = true.

I guess hard-coding it to crud=true would allow me to rename this KubernetesCrudServer. WDYT?

You definitely could, but doesn't the functionality of KubernetesServer dominate that of the KubernetesMockServer? Should the old test resource be deprecated in favor of one based upon KubernetesServer where it's easy to toggle the crud flag?

[FroMage]

You definitely could, but doesn't the functionality of KubernetesServer dominate that of the KubernetesMockServer? Should the old test resource be deprecated in favor of one based upon KubernetesServer where it's easy to toggle the crud flag?

That's an option as well.

[FroMage]

OK, pushed another version for review.

I've added support for meta-annotations to configure the test resources:

@TestProfile(KubernetesTestServerTest.MyProfile.class)
@WithKubernetesTestServer(https = false, crud = true, port = 10001)
@QuarkusTest
public class KubernetesTestServerTest {

    @KubernetesTestServer
    private KubernetesServer mockServer;

They're defined by creating an annotation with @TestResource on it:

/**
 * Use on your test resource to get a mock {@link KubernetesServer} spawn up, and injectable with {@link KubernetesTestServer}.
 * This annotation is only active when used on a test class, and only for this test class.
 */
@QuarkusTestResource(value = KubernetesServerTestResource.class, restrictToAnnotatedTest = true)
@Retention(RetentionPolicy.RUNTIME)
@Target(ElementType.TYPE)
public @interface WithKubernetesTestServer {

    /**
     * Start it with HTTPS
     */
    boolean https() default false;

    /**
     * Start it in CRUD mode
     */
    boolean crud() default true;

    /**
     * Port to use, defaults to any available port
     */
    int port() default 0;
}

And they tie into a new subtype of QuarkusTestResourceLifecycleManager:

public interface QuarkusTestResourceConfigurableLifecycleManager<ConfigAnnotation extends Annotation>
        extends QuarkusTestResourceLifecycleManager {
    default void init(ConfigAnnotation annotation) {
    }
}

Allowing easy config:

public class KubernetesServerTestResource extends AbstractKubernetesTestResource<KubernetesServer>
        implements QuarkusTestResourceConfigurableLifecycleManager<WithKubernetesTestServer> {

    private boolean https = false;
    private boolean crud = true;
    private int port = 0;

    @Override
    public void init(WithKubernetesTestServer annotation) {
        this.https = annotation.https();
        this.crud = annotation.crud();
        this.port = annotation.port();
    }

WDYT?

[FroMage]

@geoand I've made it so meta-annotations are always per-test-class but that's OK because they're new. And for existing test resources that people want to make per-test-class I've added:

public @interface QuarkusTestResource {

    /**
     * Whether this annotation should only be enabled if it is placed on the currently running test class.
     * Note that this defaults to true for meta-annotations since meta-annotations are only considered
     * for the current test class.
     */
    boolean restrictToAnnotatedTest() default false;
}

This should allow for users to work around the current behaviour without affecting existing users.

WDYT?

[FroMage]

@geoand @stuartwdouglas if you're fine with these changes I'll add docs.

[geoand]

I like the idea.

But I have a question: what happens if I write something like:

@WithKubernetesTestServer(https = false, crud = true, port = 10001)
@QuarkusTest
public class KubernetesTestServerTest {

}

Notice that I have removed the test profile.

[FroMage]

Not sure what you're trying to discover? I only use that test profile to set some config, so in terms of this PR, nothing would happen, I think.

[geoand]

What I am getting at is: "What happens when someone uses such meta-annotations on a test that does not use a profile? Does the resource get started? If, does it still stay up when the test completes?"

[FroMage]

It would get started just the same, yes, and it would get stopped as well I assume. The code I modified did not look at profiles at all, so this would be the same behaviour as if the class were annotated with @TestResource directly.

Or, are you saying this is going to be tricky if we run multiple tests at once?

[geoand]

It would get started just the same, yes, and it would get stopped as well I assume. The code I modified did not look at profiles at all, so this would be the same behaviour as if the class were annotated with @TestResource directly.

OK, that's what I wanted to know. I wanted to understand what you expect will happen.

Or, are you saying this is going to be tricky if we run multiple tests at once?

Nah, that's not really supported anyway

[geoand]

LGTM!

[FroMage]

Some reflection foiled my CI, force-pushed a fix

[FroMage]

Failing test is unrelated and flaky. Let's merge this.

[gsmet]

I don't remember why I added the triage/backport-1.11? label but... this doesn't apply cleanly at all so I think we shouldn't backport it to 1.11.

@geoand @FroMage what's your take on this?

[geoand]

This is one that @FroMage wanted, but I can't remember if he wanted it backported or not

[gsmet]

It's going to be in 1.12.1.Final that I will release in a few days. But backporting to 1.11 will require to create another PR and carefully review things. Not sure it's worth the risk.

[geoand]

We'll need Stephane to clarify that point

[gsmet]

We won't have time to do a proper backport now. Unmarking it.