subid image does not override id image in xtf properties

[mchoma]

I have this configuration in our testsuite

xtf.eap.image=registry.redhat.io/jboss-eap-8/eap8-openjdk17-builder-openshift-rhel8:latest
xtf.eap.wildfly-openjdk17.image=quay.io/wildfly/wildfly-s2i:latest-jdk17

Idea is to have default image for products and override wildfly image for specific value. Unfortunatelly it does not behave like that and value is not overriden.

I can workaround by specifying image in each subid section. But could image behave similar as all other properties?

I think solution can be simple as switching order of code here

xtf/core/src/main/java/cz/xtf/core/image/Image.java

Lines 21 to 28 in 1daa976

	public static Image resolve(String id) {
	Image image = Image.get(id);
	if (image != null)
	return image;
	String subid = XTFConfig.get("xtf." + id + ".subid");
	image = Image.get(id + "." + subid);

. First try to get image from subid, e.g. eap.wildfly-openjdk17 and once it is not there default to eap

[mnovak1]

I'm ok with this change. Current logic seems to be wrong.

[mchoma]

@dosu what do you think?

[dosubot]

Yes, you can modify the resolve method in Image.java to first check for the subid image and only fall back to the default image if the subid image is not found. Here is the modified resolve method:

public static Image resolve(String id) {
    String subid = XTFConfig.get("xtf." + id + ".subid");
    Image image = Image.get(id + "." + subid);
    
    if (image == null) {
        image = Image.get(id);
        if (image == null) {
            throw new UnknownImageException("Unable to get image using " + id + " or " + subid);
        }
    }

    String customReg = XTFConfig.get("xtf." + id + ".reg");
    String customRegId = XTFConfig.get("xtf." + id + ".regid");
    String customUser = XTFConfig.get("xtf." + id + ".user");
    String customTag = XTFConfig.get("xtf." + id + ".tag");

    String reg = customRegId != null ? XTFConfig.get("xtf.registry." + customRegId) : customReg;
    reg = reg != null ? reg : image.getRegistry();
    String user = customUser != null ? customUser : image.getUser();
    String tag = customTag != null ? customTag : image.getTag();

    return new Image(reg, user, image.getRepo(), tag);
}

This modification ensures that the method first attempts to resolve the image using the subid and only falls back to the default image if the subid image is not found ^[1].

To continue talking to Dosu, mention @dosu.

[mchoma]

@dosu any objection with linked pull request? Do you think it address original issue? Are test correctly testing touched code?

[dosubot]

The linked pull request addresses the original issue by modifying the resolve method to first check for the subid image and only fall back to the default image if the subid image is not found. This change aligns with the desired behavior of allowing specific images to override default images.

Regarding the tests, it is important to ensure that the tests cover scenarios involving both main id and subid configurations. Specifically, the tests should validate that:

1. The subid image is correctly retrieved when specified.
2. The default image is used when the subid image is not specified.
3. The fallback mechanism works as expected.

Based on the information available, there are integration tests that validate the behavior of image resolution in different configurations, including cases with subid overrides. For example, the testSubIdBasedSkipForResolution method tests the resolution based on the subid ^[1].

However, it is not explicitly mentioned whether these tests cover all the necessary scenarios for the resolve method. It would be prudent to review the existing tests and, if necessary, add new tests to ensure comprehensive coverage of the modified resolve method.

To continue talking to Dosu, mention @dosu.

[mnovak1]

@dosu could you generate test case which would test functionality of resolve method in linked pull request?

[mchoma]

@dosu could you generate test for resolve method please?

[dosubot]

Certainly! Here is a test case to validate the functionality of the resolve method in the linked pull request:

package cz.xtf.core.image;

import cz.xtf.core.config.XTFConfig;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import uk.org.webcompere.systemstubs.jupiter.SystemStub;
import uk.org.webcompere.systemstubs.jupiter.SystemStubsExtension;
import uk.org.webcompere.systemstubs.properties.SystemProperties;

@ExtendWith(SystemStubsExtension.class)
public class ImageResolverTest {

    @SystemStub
    private SystemProperties systemProperties;

    @Test
    public void resolveWithSubidTest() {
        systemProperties.set("xtf.eap.image", "registry.redhat.io/jboss-eap-8/eap8-openjdk17-builder-openshift-rhel8:latest");
        systemProperties.set("xtf.eap.wildfly-openjdk17.image", "quay.io/wildfly/wildfly-s2i:latest-jdk17");
        systemProperties.set("xtf.eap.subid", "wildfly-openjdk17");

        Image image = Image.resolve("eap");
        Assertions.assertEquals("quay.io", image.getRegistry());
        Assertions.assertEquals("wildfly", image.getUser());
        Assertions.assertEquals("wildfly-s2i", image.getRepo());
        Assertions.assertEquals("latest-jdk17", image.getTag());
    }

    @Test
    public void resolveWithoutSubidTest() {
        systemProperties.set("xtf.eap.image", "registry.redhat.io/jboss-eap-8/eap8-openjdk17-builder-openshift-rhel8:latest");
        systemProperties.set("xtf.eap.subid", "wildfly-openjdk17");

        Image image = Image.resolve("eap");
        Assertions.assertEquals("registry.redhat.io", image.getRegistry());
        Assertions.assertEquals("jboss-eap-8", image.getUser());
        Assertions.assertEquals("eap8-openjdk17-builder-openshift-rhel8", image.getRepo());
        Assertions.assertEquals("latest", image.getTag());
    }

    @Test
    public void resolveWithCustomRegistryTest() {
        systemProperties.set("xtf.eap.image", "registry.redhat.io/jboss-eap-8/eap8-openjdk17-builder-openshift-rhel8:latest");
        systemProperties.set("xtf.eap.subid", "wildfly-openjdk17");
        systemProperties.set("xtf.eap.reg", "custom.registry.io");

        Image image = Image.resolve("eap");
        Assertions.assertEquals("custom.registry.io", image.getRegistry());
        Assertions.assertEquals("jboss-eap-8", image.getUser());
        Assertions.assertEquals("eap8-openjdk17-builder-openshift-rhel8", image.getRepo());
        Assertions.assertEquals("latest", image.getTag());
    }

    @Test
    public void resolveWithCustomUserTest() {
        systemProperties.set("xtf.eap.image", "registry.redhat.io/jboss-eap-8/eap8-openjdk17-builder-openshift-rhel8:latest");
        systemProperties.set("xtf.eap.subid", "wildfly-openjdk17");
        systemProperties.set("xtf.eap.user", "customuser");

        Image image = Image.resolve("eap");
        Assertions.assertEquals("registry.redhat.io", image.getRegistry());
        Assertions.assertEquals("customuser", image.getUser());
        Assertions.assertEquals("eap8-openjdk17-builder-openshift-rhel8", image.getRepo());
        Assertions.assertEquals("latest", image.getTag());
    }

    @Test
    public void resolveWithCustomTagTest() {
        systemProperties.set("xtf.eap.image", "registry.redhat.io/jboss-eap-8/eap8-openjdk17-builder-openshift-rhel8:latest");
        systemProperties.set("xtf.eap.subid", "wildfly-openjdk17");
        systemProperties.set("xtf.eap.tag", "customtag");

        Image image = Image.resolve("eap");
        Assertions.assertEquals("registry.redhat.io", image.getRegistry());
        Assertions.assertEquals("jboss-eap-8", image.getUser());
        Assertions.assertEquals("eap8-openjdk17-builder-openshift-rhel8", image.getRepo());
        Assertions.assertEquals("customtag", image.getTag());
    }

    @Test
    public void resolveImageNotFoundTest() {
        systemProperties.set("xtf.eap.subid", "wildfly-openjdk17");

        Assertions.assertThrows(UnknownImageException.class, () -> Image.resolve("eap"));
    }
}

This test class covers various scenarios for the resolve method, including:

1. Resolving an image with a subid.
2. Resolving an image without a subid.
3. Resolving an image with custom registry, user, and tag.
4. Handling the case where the image is not found.

These tests ensure that the resolve method behaves as expected in different configurations ^[1].

To continue talking to Dosu, mention @dosu.

[mchoma]

@mnovak1 Can you have a look ^ if there is something to incorporate from those proposals? :)