Backwards compatibility fix for importing TF versions by number

[sebasslash]

Older TFE installations don't support the filter[version] query param. This commit updates
the importer function to fallback to looping through versions to find a match should the API
not support the filter query param.

[brandonc]

You could structure this to linear search all results without re-requesting the first page:

Suggested change

	if versions.Items[0].Version != version {
	options := tfe.AdminTerraformVersionsListOptions{}
	found:
	for {
	vlist, err := client.Admin.TerraformVersions.List(ctx, options)
	if err != nil {
	return "", fmt.Errorf("error reading Terraform Versions: %w", err)
	}
	if vlist.CurrentPage >= vlist.TotalPages {
	break
	}
	for _, v := range vlist.Items {
	if v.Version == version {
	versionID = v.ID
	break found
	}
	}
	options.PageNumber = vlist.NextPage
	}
	} else {
	versionID = versions.Items[0].ID
	}
	if versionID == "" {
	return "", fmt.Errorf("terraform version not found")
	}
	return versionID, nil
	if len(versions.Items) > 1 {
	options := tfe.AdminTerraformVersionsListOptions{}
	found:
	for {
	for _, v := range versions {
	if v.Version == version {
	versionID = v.ID
	break found
	}
	}
	if versions.CurrentPage >= versions.TotalPages {
	break
	}
	options.PageNumber = versions.NextPage
	versions, err = client.Admin.TerraformVersions.List(ctx, options)
	if err != nil {
	return "", fmt.Errorf("error reading Terraform Versions: %w", err)
	}
	}
	} else if versions.Items[0].Version == version {
	versionID = versions.Items[0].ID
	}
	if versionID == "" {
	return "", fmt.Errorf("terraform version not found")
	}
	return versionID, nil

[brandonc]

(I didn't compile or test this code)

[nfagerlund]

A couple of things!

• One early-bail glitch noted inline.
• Echoing Brandon's comment about skipping the redundant HTTP call, except also I think you don't need the outer if statement at all -- the "happy path" case of a recent TFC/TFE version is actually doing the exact same thing as the "sad path" case, it's just that the version it's looking for will always happen to be the first and only thing in the list, so the same double loop can just handle everything.
• Separately from all of that, I think it's worth adding a comment about why we're doing this! Just something tiny'll do -- // This is usually a one-item list due to the filter parameter, but TFE versions <= 2022.01 ignore it and return everything, so we have to scan the list.

[nfagerlund]

I think you've put this too early! It should go after the inner search loop, otherwise it's impossible to ever find a version that happens to be on the final page of the list. Cf. prior art in readWorkspaceStateConsumers, for example.

[brandonc]

We talked about this in person, but one corner case is that if a TFE installation prior to the filter API has only one version available, the case 1 stanza would be incorrect. We can fix this by doing a version compare and returning an error

[nfagerlund]

I agree with Brandon's comment, but assuming you deal with that corner case, I'm good with this refactor! I still think the case 1 technically isn't necessary, but I suppose it does help make the code reflect the way we're thinking about this problem, and I'm fine with redundancy as long as it's at least explanatory. 👍🏼