[TF-24744] Terraform Version Arm Support #1714
Conversation
There was a problem hiding this comment.
This looks good. Almost all of my comments are about the same thing - I'm thinking we should let Atlas handle top level URL/SHA vs Archs decisions. That being said, I could be convinced of otherwise. Let me know your thoughts. This could have been clearer in the ticket so apologies there.
I added a bullet point to the A/C for a deprecation message.
| Type: schema.TypeString, | ||
| Optional: true, | ||
| Default: nil, | ||
| ConflictsWith: []string{"archs"}, |
There was a problem hiding this comment.
If I am understanding ConflictsWith properly, it cannot be used w/ "archs" at all, right? I think it should be allowed to be used w/ archs because maybe there are url, sha, and arm64 arch fields or url, sha, and matching amd64 fields.
There was a problem hiding this comment.
yeah, I can definitely update this validation. I added this when I was having weird behavior with the api. But with TFDN-842 this behavior should be taken care of
| Optional: true, | ||
| Default: nil, | ||
| ConflictsWith: []string{"archs"}, | ||
| RequiredWith: []string{"sha"}, |
There was a problem hiding this comment.
For PATCH requests, a SHA does not have to be present w/ a URL and the other way around. Will this interfere with that?
| Optional: true, | ||
| Default: nil, | ||
| ConflictsWith: []string{"url", "sha"}, | ||
| AtLeastOneOf: []string{"archs", "url", "sha"}, |
There was a problem hiding this comment.
Still wondering about the PATCH here. Let me know if you want me to do any manual testing around this.
| } else { | ||
| url, urlOk := d.GetOk("url") | ||
| sha, shaOk := d.GetOk("sha") | ||
| if urlOk && shaOk { | ||
| opts.URL = tfe.String(url.(string)) | ||
| opts.Sha = tfe.String(sha.(string)) | ||
| } | ||
| } |
There was a problem hiding this comment.
I can see why you have an else because we want to get away from relying on URL and SHA but I wonder if we should set the URL and SHA if present regardless of Archs because then the user will get the 422 if they don't match. It might be confusing for them to send these values and not get any acknowledgement of them.
There was a problem hiding this comment.
Ok, the reason that I had conditionally set these was because of this warning that is returned that may be confusing (but may also be acceptable, I am not familiar with providers 😅 )
This was how I set the tfe_terraform_version:
resource "tfe_terraform_version" "test" {
version = "9.9.9"
deprecated = false
enabled = true
official = false
beta = false
archs {
url = "https://releases.hashicorp.com/terraform/1.11.0-alpha20241211/terraform_1.11.0-alpha20241211_linux_amd64.zip"
sha = "dd68a600989c122c5e2ca9fd055f071bfbff8fa7047b48349489e4290b62b8c1"
os = "linux"
arch = "amd64"
}
archs {
url = "https://releases.hashicorp.com/terraform/1.11.0-alpha20241211/terraform_1.11.0-alpha20241211_linux_amd64.zip"
sha = "dd68a600989c122c5e2ca9fd055f071bfbff8fa7047b48349489e4290b62b8c9"
os = "linux"
arch = "arm64"
}
}
Since the tool_version url and sha are sent back at the read, it set's both of those values in state and returns this warning:
2025-05-02T10:14:31.067-0600 [WARN] Provider "provider[\"registry.terraform.io/hashicorp/tfe\"]" produced an unexpected new value for tfe_terraform_version.test, but we are tolerating it because it is using the legacy plugin SDK.
The following problems may be the cause of any confusing errors from downstream operations:
- .url: was null, but now cty.StringVal("https://releases.hashicorp.com/terraform/1.11.0-alpha20241211/terraform_1.11.0-alpha20241211_linux_amd64.zip")
- .sha: was null, but now cty.StringVal("dd68a600989c122c5e2ca9fd055f071bfbff8fa7047b48349489e4290b62b8c1")
There was a problem hiding this comment.
I updated to just set it each time, since it is a lot cleaner. But curious on your thoughts!
| d.Set("url", v.URL) | ||
| d.Set("sha", v.Sha) |
There was a problem hiding this comment.
I'm wondering if we should leave these and realizing I should not have included details like "replace" in the ticket. I'll fix that.
| d.Set("url", nil) | ||
| d.Set("sha", nil) | ||
| } else { | ||
| d.Set("archs", nil) |
There was a problem hiding this comment.
| d.Set("archs", nil) |
This will default to nil
| d.Set("url", nil) | ||
| d.Set("sha", nil) |
There was a problem hiding this comment.
Maybe we populate these and let Atlas handle it.
| URL: tfe.String(d.Get("url").(string)), | ||
| Sha: tfe.String(d.Get("sha").(string)), |
There was a problem hiding this comment.
Same as lines 78, 79.
Description
tfe_terraform_versionresource to havearchsurlandshaORarchsis requiredarchson update/create opts (if present)Remember to:
Testing plan
External links
Include any links here that might be helpful for people reviewing your PR. If there are none, feel free to delete this section.
Output from acceptance tests
Please run applicable acceptance tests locally and include the output here. See testing.md to learn how to run acceptance tests.
If you are an external contributor, your contribution(s) will first be reviewed before running them against the project's CI pipeline.