fix(repositories): Handle download_concurrency missing API return val…

…ue (#177)

Commit introduces multiple fixes to handle the missing
"download_concurrency" parameter in Katello API responses. The func
handleDownloadConcurrencyBetweenTerraformAndKatello basically compares
the API response with the state, so if the API did not return the
value, but we know the value from the Terraform state, we can override
this value in the repository struct.

Also adds validation and a fix for the ContentViewFilter query, which
seems to have been missed in previous test runs.

Resolves #161

Signed-off-by: Dominik Pataky <[email protected]>
Co-authored-by: Lennart Weller <[email protected]>

foreman/api/katello_content_views.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"github.com/terraform-coop/terraform-provider-foreman/foreman/utils"
 	"net/http"
@@ -250,6 +251,36 @@ func (c *Client) ReadKatelloContentView(ctx context.Context, d *ContentView) (*C
 	return &cv, nil
 }
 
+func (c *Client) ReadContentViewFilters(ctx context.Context, cvId int) (*[]ContentViewFilter, error) {
+	utils.TraceFunctionCall()
+
+	reqEndpoint := fmt.Sprintf(ContentViewFilters, cvId)
+	var cvfs []ContentViewFilter
+	var queryResponse QueryResponse
+
+	req, err := c.NewRequestWithContext(ctx, http.MethodGet, reqEndpoint, nil)
+	if err != nil {
+		return nil, err
+	}
+
+	err = c.SendAndParse(req, &queryResponse)
+	if err != nil {
+		return nil, err
+	}
+
+	for _, item := range queryResponse.Results {
+		cvf, ok := item.(ContentViewFilter)
+		if !ok {
+			return nil, errors.New("failed to cast content view filter from results[]")
+		}
+		cvfs = append(cvfs, cvf)
+	}
+
+	utils.Debugf("read content_view filter: %+v", cvfs)
+
+	return &cvfs, nil
+}
+
 func (c *Client) UpdateKatelloContentView(ctx context.Context, cv *ContentView) (*ContentView, error) {
 	utils.TraceFunctionCall()
 

foreman/api/repository.go

@@ -69,23 +69,32 @@ type ForemanKatelloRepository struct {
 
 func (r *ForemanKatelloRepository) MarshalJSON() ([]byte, error) {
 	m := map[string]interface{}{
-		"id":                   r.Id,
-		"name":                 r.Name,
-		"description":          r.Description,
-		"label":                r.Label,
-		"product_id":           r.ProductId,
-		"url":                  r.Url,
-		"unprotected":          r.Unprotected,
-		"checksum_type":        r.ChecksumType,
-		"ignore_global_proxy":  r.IgnoreGlobalProxy,
-		"ignorable_content":    r.IgnorableContent,
-		"download_policy":      r.DownloadPolicy,
-		"download_concurrency": r.DownloadConcurrency,
-		"mirroring_policy":     r.MirroringPolicy,
-		"mirror_on_sync":       r.MirrorOnSync, // deprecated
-		"verify_ssl_on_sync":   r.VerifySslOnSync,
-		"upstream_username":    r.UpstreamUsername,
-		"upstream_password":    r.UpstreamPassword,
+		"id":                  r.Id,
+		"name":                r.Name,
+		"description":         r.Description,
+		"label":               r.Label,
+		"product_id":          r.ProductId,
+		"url":                 r.Url,
+		"unprotected":         r.Unprotected,
+		"checksum_type":       r.ChecksumType,
+		"ignore_global_proxy": r.IgnoreGlobalProxy,
+		"ignorable_content":   r.IgnorableContent,
+		"download_policy":     r.DownloadPolicy,
+		"mirroring_policy":    r.MirroringPolicy,
+		"mirror_on_sync":      r.MirrorOnSync, // deprecated
+		"verify_ssl_on_sync":  r.VerifySslOnSync,
+		"upstream_username":   r.UpstreamUsername,
+		"upstream_password":   r.UpstreamPassword,
+	}
+
+	// Creating a repository with download_concurrency > 0 works, but the Katello API
+	// does not return this value. Therefore Terraform stores the resource with
+	// download_concurrency=0 in the state, passing 0 in on resource updates. But
+	// this does not work, since the API expects download_concurrency>0, therefore we
+	// skip this field entirely if we run into this case in the JSON marshalling
+	// step.
+	if r.DownloadConcurrency != 0 {
+		m["download_concurrency"] = r.DownloadConcurrency
 	}
 
 	m["content_type"] = r.ContentType

foreman/resource_foreman_katello_repositories.go

@@ -2,6 +2,7 @@ package foreman
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"strconv"
 
@@ -165,6 +166,7 @@ func resourceForemanKatelloRepository() *schema.Resource {
 						"Use value less than 20. Defaults to 10. Warning: the value is not returned from the API and " +
 						"is therefore handled by a DiffSuppressFunc.",
 				),
+				ValidateFunc: validation.IntBetween(1, 20),
 				DiffSuppressFunc: func(key, oldValue, newValue string, d *schema.ResourceData) bool {
 					// "download_concurrency" is not returned from the Katello API, but still exists in the
 					// source code at https://github.com/Katello/katello/blob/6d8d3ca36e1469d1f7c2c8e180e42467176ac1a4/app/controllers/katello/api/v2/repositories_controller.rb#L56.
@@ -284,6 +286,7 @@ func resourceForemanKatelloRepository() *schema.Resource {
 			"http_proxy_policy": {
 				Type:     schema.TypeString,
 				Optional: true,
+				Default:  "global_default_http_proxy",
 				ValidateFunc: validation.StringInSlice([]string{
 					"global_default_http_proxy",
 					"none",
@@ -372,7 +375,12 @@ func setResourceDataFromForemanKatelloRepository(d *schema.ResourceData, repo *a
 	d.Set("docker_upstream_name", repo.DockerUpstreamName)
 	d.Set("docker_tags_whitelist", repo.DockerTagsWhitelist)
 	d.Set("download_policy", repo.DownloadPolicy)
-	d.Set("download_concurrency", repo.DownloadConcurrency)
+
+	if repo.DownloadConcurrency > 0 {
+		// In case it is 0 and unset, the value will default
+		d.Set("download_concurrency", repo.DownloadConcurrency)
+	}
+
 	d.Set("mirror_on_sync", repo.MirrorOnSync)
 	d.Set("mirroring_policy", repo.MirroringPolicy)
 	d.Set("verify_ssl_on_sync", repo.VerifySslOnSync)
@@ -392,6 +400,27 @@ func setResourceDataFromForemanKatelloRepository(d *schema.ResourceData, repo *a
 // Resource CRUD Operations
 // -----------------------------------------------------------------------------
 
+func handleDownloadConcurrencyBetweenTerraformAndKatello(resData *schema.ResourceData, repo *api.ForemanKatelloRepository) error {
+	log.Tracef("handleDownloadConcurrencyBetweenTerraformAndKatello")
+
+	// Handle missing download_concurrency attribute in API response
+	originalDLC, ok := resData.GetOk("download_concurrency")
+	if ok {
+		originalDLCint, ok := originalDLC.(int)
+
+		if !ok {
+			return errors.New("unable to convert 'download_concurrency' state value to int")
+		}
+
+		if originalDLCint > 0 && repo.DownloadConcurrency == 0 {
+			// We passed in a value > 0, but the Katello API did not return this value and therefore the default 0 was applied to the struct
+			log.Debugf("State has download_concurrency of %d, but repo has 0. Setting repo object parameter to state", originalDLCint)
+			repo.DownloadConcurrency = originalDLCint
+		}
+	}
+	return nil
+}
+
 func resourceForemanKatelloRepositoryCreate(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics {
 	log.Tracef("resource_foreman_katello_repository.go#Create")
 
@@ -405,6 +434,11 @@ func resourceForemanKatelloRepositoryCreate(ctx context.Context, d *schema.Resou
 		return diag.FromErr(createErr)
 	}
 
+	err := handleDownloadConcurrencyBetweenTerraformAndKatello(d, createdKatelloRepository)
+	if err != nil {
+		return diag.FromErr(err)
+	}
+
 	log.Debugf("Created ForemanKatelloRepository: [%+v]", createdKatelloRepository)
 
 	setResourceDataFromForemanKatelloRepository(d, createdKatelloRepository)
@@ -425,6 +459,11 @@ func resourceForemanKatelloRepositoryRead(ctx context.Context, d *schema.Resourc
 		return diag.FromErr(api.CheckDeleted(d, readErr))
 	}
 
+	err := handleDownloadConcurrencyBetweenTerraformAndKatello(d, readKatelloRepository)
+	if err != nil {
+		return diag.FromErr(err)
+	}
+
 	log.Debugf("Read ForemanKatelloRepository: [%+v]", readKatelloRepository)
 
 	setResourceDataFromForemanKatelloRepository(d, readKatelloRepository)
@@ -445,6 +484,11 @@ func resourceForemanKatelloRepositoryUpdate(ctx context.Context, d *schema.Resou
 		return diag.FromErr(updateErr)
 	}
 
+	err := handleDownloadConcurrencyBetweenTerraformAndKatello(d, updatedKatelloRepository)
+	if err != nil {
+		return diag.FromErr(err)
+	}
+
 	log.Debugf("ForemanKatelloRepository: [%+v]", updatedKatelloRepository)
 
 	setResourceDataFromForemanKatelloRepository(d, updatedKatelloRepository)