-
Notifications
You must be signed in to change notification settings - Fork 43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HCS-1760: Add Consul versions data source #63
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two things.
First is should we expose preview versions in the provider before we support them in HCP itself? If we wanted to call them something other than preview in the future once we went down the path of implementing them then it would potentially be a breaking change to the provider.
Secondly the doc comment I pointed out doesn't match the func name. However the name in the comment is referenced from new code so I am not too sure whats going on here.
Great point @mkeeler. Fortunately this implementation is tied to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, my comment on the id used here should not be blocking
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just had one comment that might make the ids more robust
internal/consul/version.go
Outdated
return strings.Join(otherVersions, ", ") | ||
} | ||
|
||
return fmt.Sprintf("%s, %s", recommendedVersion, strings.Join(otherVersions, ", ")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be weird to prepend this string with something like recommended:
(and same for line 61)? It might be overkill, but I think there's an edge case where eg. v1.9.0, v1.8.6, v1.8.4
would be produced by two configurations: no recommended version, and v1.9.0
recommended version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can definitely get behind adding it for this line. On 61 since there would only be 1 version returned (the recommended one) it seems like overkill 🤷
Based off the
hcs_consul_versions
data source, we can allows users to explicitly set their Consul cluster version to the latest recommended Consul version that HCP supports. This means that any time the recommended HCP Consul version changes, someone utilizing therecommended
field of theconsul_versions
data source will be prompted to upgrade their cluster the next time they runterraform plan
:This data source also informs users of the Consul versions that HCP supports, in the case where they are looking to use a specific Consul version.
If HCP offers preview versions of Consul in the future, users would be able to discover and set
min_consul_version
of their cluster using this data source.Usage: