✨ Improve image filtering (#124)

* Improve image filtering

Signed-off-by: michal.gubricky <[email protected]>

* Return empty string if image with ID is not found

Signed-off-by: michal.gubricky <[email protected]>

* Update internal/controller/openstacknodeimagerelease_controller.go

Co-authored-by: Matej Feder <[email protected]>
Signed-off-by: Michal Gubricky <[email protected]>

* Fix unit-tests after added suggestion

Signed-off-by: michal.gubricky <[email protected]>

---------

Signed-off-by: michal.gubricky <[email protected]>
Signed-off-by: Michal Gubricky <[email protected]>
Co-authored-by: Matej Feder <[email protected]>

Author: michal-gubricky

internal/controller/openstacknodeimagerelease_controller.go

@@ -147,7 +147,7 @@ func (r *OpenStackNodeImageReleaseReconciler) Reconcile(ctx context.Context, req
 
 	conditions.MarkTrue(openstacknodeimagerelease, apiv1alpha1.OpenStackImageServiceClientAvailableCondition)
 
-	imageID, err := findImageByName(imageClient, openstacknodeimagerelease.Spec.Image.CreateOpts.Name)
+	imageID, err := getImageID(imageClient, openstacknodeimagerelease.Spec.Image.CreateOpts)
 	if err != nil {
 		conditions.MarkFalse(openstacknodeimagerelease,
 			apiv1alpha1.OpenStackImageReadyCondition,
@@ -331,27 +331,38 @@ func (r *OpenStackNodeImageReleaseReconciler) getCloudFromSecret(ctx context.Con
 	return cloud, nil
 }
 
-func findImageByName(imagesClient *gophercloud.ServiceClient, imageName string) (string, error) {
-	listOpts := images.ListOpts{
-		Name: imageName,
+func getImageID(imagesClient *gophercloud.ServiceClient, imageCreateOps *apiv1alpha1.CreateOpts) (string, error) {
+	var listOpts images.ListOpts
+
+	if imageCreateOps.ID != "" {
+		listOpts = images.ListOpts{
+			ID: imageCreateOps.ID,
+		}
+	} else {
+		listOpts = images.ListOpts{
+			Name: imageCreateOps.Name,
+			Tags: imageCreateOps.Tags,
+		}
 	}
 
 	allPages, err := images.List(imagesClient, listOpts).AllPages()
 	if err != nil {
-		return "", fmt.Errorf("failed to list images with name %s: %w", imageName, err)
+		return "", fmt.Errorf("failed to list images with name %s: %w", imageCreateOps.Name, err)
 	}
 
 	imageList, err := images.ExtractImages(allPages)
 	if err != nil {
-		return "", fmt.Errorf("failed to extract images with name %s: %w", imageName, err)
+		return "", fmt.Errorf("failed to extract images with name %s: %w", imageCreateOps.Name, err)
 	}
 
-	for i := range imageList {
-		if imageList[i].Name == imageName {
-			return imageList[i].ID, nil
-		}
+	switch len(imageList) {
+	case 0:
+		return "", nil
+	case 1:
+		return imageList[0].ID, nil
+	default:
+		return "", fmt.Errorf("too many images were found with the given image name: %s and tags: %s", imageCreateOps.Name, imageCreateOps.Tags)
 	}
-	return "", nil
 }
 
 func createImage(imageClient *gophercloud.ServiceClient, createOpts *apiv1alpha1.CreateOpts) (*images.Image, error) {

internal/controller/openstacknodeimagerelease_controller_test.go

@@ -188,15 +188,34 @@ clouds:
 	assert.NoError(t, err)
 }
 
-func TestFindImageByName(t *testing.T) {
+func TestGetImageID(t *testing.T) {
 	th.SetupHTTP()
 	defer th.TeardownHTTP()
 
 	HandleImageListSuccessfully(t)
 
-	imageName := "test_image"
+	imageFilter := &apiv1alpha1.CreateOpts{
+		ID: "123",
+	}
+
+	imageID, err := getImageID(fakeclient.ServiceClient(), imageFilter)
+
+	assert.NoError(t, err)
+	assert.Equal(t, "123", imageID)
+}
+
+func TestGetImageIDByNameAndTags(t *testing.T) {
+	th.SetupHTTP()
+	defer th.TeardownHTTP()
+
+	HandleImageListSuccessfully(t)
+
+	imageFilter := &apiv1alpha1.CreateOpts{
+		Name: "test_image",
+		Tags: []string{"v1"},
+	}
 
-	imageID, err := findImageByName(fakeclient.ServiceClient(), imageName)
+	imageID, err := getImageID(fakeclient.ServiceClient(), imageFilter)
 
 	assert.NoError(t, err)
 	assert.Equal(t, "123", imageID)
@@ -213,33 +232,103 @@ func HandleImageListSuccessfully(t *testing.T) { //nolint: gocritic
 		w.WriteHeader(http.StatusOK)
 		fmt.Fprintf(w, `{
 			"images": [
-				{"id": "123", "name": "test_image"},
-				{"id": "456", "name": "test_image2"},
-				{"id": "789", "name": "test_image3"}
+				{"id": "123", "name": "test_image", "tags": ["v1"]}
+			]
+		}`)
+	})
+}
+
+func TestGetImageIDWithTwoSameImageNames(t *testing.T) {
+	th.SetupHTTP()
+	defer th.TeardownHTTP()
+
+	HandleImageListWithTwoImagesSuccessfully(t)
+
+	imageFilter := &apiv1alpha1.CreateOpts{
+		Name: "test_image",
+		Tags: []string{"v1"},
+	}
+
+	imageID, err := getImageID(fakeclient.ServiceClient(), imageFilter)
+
+	assert.Error(t, err) // Expecting an error due to multiple images with the same name
+	assert.Equal(t, "", imageID)
+	assert.Equal(t, err.Error(), "too many images were found with the given image name: test_image and tags: [v1]")
+}
+
+// HandleImageListWithTwoImagesSuccessfully sets up a fake response for image list request.
+func HandleImageListWithTwoImagesSuccessfully(t *testing.T) { //nolint: gocritic
+	t.Helper() // Indicate that this is a test helper function
+	th.Mux.HandleFunc("/images", func(w http.ResponseWriter, r *http.Request) {
+		th.TestMethod(t, r, "GET")
+		th.TestHeader(t, r, "X-Auth-Token", fakeclient.TokenID)
+
+		w.Header().Set("Content-Type", "application/json")
+		w.WriteHeader(http.StatusOK)
+		fmt.Fprintf(w, `{
+			"images": [
+				{"id": "123", "name": "test_image", "tags": ["v1"]},
+				{"id": "456", "name": "test_image", "tags": ["v1"]}
 			]
 		}`)
 	})
 }
 
-func TestFindImageByNameWrongImageName(t *testing.T) {
+func TestGetImageIDNoImageFound(t *testing.T) {
+	th.SetupHTTP()
+	defer th.TeardownHTTP()
+
+	HandleImageListWithNoImageFoundSuccessfully(t)
+
+	imageFilter := &apiv1alpha1.CreateOpts{
+		Name: "test_image",
+		Tags: []string{"v1"},
+	}
+
+	imageID, err := getImageID(fakeclient.ServiceClient(), imageFilter)
+
+	assert.NoError(t, err)
+	assert.Equal(t, "", imageID)
+}
+
+// HandleImageListWithNoImageFoundSuccessfully sets up a fake response for image list request.
+func HandleImageListWithNoImageFoundSuccessfully(t *testing.T) { //nolint: gocritic
+	t.Helper() // Indicate that this is a test helper function
+	th.Mux.HandleFunc("/images", func(w http.ResponseWriter, r *http.Request) {
+		th.TestMethod(t, r, "GET")
+		th.TestHeader(t, r, "X-Auth-Token", fakeclient.TokenID)
+
+		w.Header().Set("Content-Type", "application/json")
+		w.WriteHeader(http.StatusOK)
+		fmt.Fprintf(w, `{
+			"images": []
+		}`)
+	})
+}
+
+func TestGetImageIDWrongImageName(t *testing.T) {
 	th.SetupHTTP()
 	defer th.TeardownHTTP()
 
 	HandleImageListSuccessfully(t)
 
-	imageName := "test_bad_image"
+	imageFilter := &apiv1alpha1.CreateOpts{
+		Name: "test_bad_image",
+	}
 
-	imageID, err := findImageByName(fakeclient.ServiceClient(), imageName)
+	imageID, err := getImageID(fakeclient.ServiceClient(), imageFilter)
 
 	assert.NoError(t, err)
 	assert.NotEqual(t, "231", imageID)
 }
 
-func TestFindImageByNameNotFound(t *testing.T) {
+func TestGetImageIDNotFound(t *testing.T) {
 	th.SetupHTTP()
 	defer th.TeardownHTTP()
 
-	imageName := "test_image"
+	imageFilter := &apiv1alpha1.CreateOpts{
+		Name: "test_image",
+	}
 
 	th.Mux.HandleFunc("/images", func(w http.ResponseWriter, r *http.Request) {
 		th.TestMethod(t, r, http.MethodGet)
@@ -253,11 +342,11 @@ func TestFindImageByNameNotFound(t *testing.T) {
 
 	fakeClient := fakeclient.ServiceClient()
 
-	imageID, err := findImageByName(fakeClient, imageName)
+	imageID, err := getImageID(fakeClient, imageFilter)
 
 	assert.Error(t, err)
 	assert.Equal(t, "", imageID)
-	assert.Contains(t, err.Error(), fmt.Sprintf("failed to list images with name %s: Resource not found", imageName))
+	assert.Contains(t, err.Error(), fmt.Sprintf("failed to list images with name %s: Resource not found", imageFilter.Name))
 }
 
 // HandleImageCreationSuccessfully test setup.