Skip to content
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

[TF-18527] Add Archs to AdminTerraformVersionCreateOptions #1022

Merged

Conversation

natalie-todd
Copy link
Contributor

@natalie-todd natalie-todd commented Dec 20, 2024

Description

The purpose of this PR is to update the AdminTerraformVersionCreateOptions struct with an Archs field so Terraform Releases can add Terraform versions with Linux amd64 and arm64 architectures.

External links

@natalie-todd natalie-todd requested a review from a team as a code owner December 20, 2024 18:41
@datadog-terraform-cloud-hashicorp
Copy link

datadog-terraform-cloud-hashicorp bot commented Dec 20, 2024

Datadog Report

Branch report: ntodd/tf-18527/support-tool-version-architectures
Commit report: bf8719c
Test service: hashicorp/go-tfe

✅ 0 Failed, 1419 Passed, 153 Skipped, 21m 47.33s Total Time
➡️ Test Sessions change in coverage: 1 no change

Copy link
Contributor

@sebasslash sebasslash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding this! A few blocking things before we merge:

  • We should add tests to admin_terraform_version_integration_test.go to ensure this new field is behaving as expected when we create/update a version.
  • Please add a changelog entry under the # Unreleased section
  • See comment below

Thank you 👍

admin_terraform_version.go Outdated Show resolved Hide resolved
@natalie-todd natalie-todd force-pushed the ntodd/tf-18527/support-tool-version-architectures branch from 2dadf41 to b3dc95d Compare December 31, 2024 19:02
@natalie-todd
Copy link
Contributor Author

Thanks for adding this! A few blocking things before we merge:

  • We should add tests to admin_terraform_version_integration_test.go to ensure this new field is behaving as expected when we create/update a version.
  • Please add a changelog entry under the # Unreleased section
  • See comment below

Thank you 👍

Hi @sebasslash, I think I addressed these bullet points but let me know if there is anything else I'm missing. Thanks!

@natalie-todd natalie-todd requested review from a team and radditude January 2, 2025 22:08
radditude
radditude previously approved these changes Jan 2, 2025
Copy link
Member

@radditude radditude left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of very minor questions, but this makes sense to me as a first pass to enable basic usage of the create endpoint while the feature is still flagged!

CHANGELOG.md Outdated
* Add support for project level auto destroy settings @simonxmh [#1011](https://github.com/hashicorp/go-tfe/pull/1011)
* Add `Archs` field to `AdminTerraformVersionCreateOptions` by @natalie-todd [#1022](https://github.com/hashicorp/go-tfe/pull/1022)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question for core cloud folks: do we need to note that this is in beta to give any TFE users a heads up that they won't be able to use that field yet?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, yes! In the past we have used words like: Adds BETA support for [such and such feature], which is EXPERIMENTAL, SUBJECT TO CHANGE, and may not be available to all users.
Those words could be applied here.

@@ -194,7 +203,7 @@ func (a *adminTerraformVersions) Delete(ctx context.Context, id string) error {
}

func (o AdminTerraformVersionCreateOptions) valid() error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to add a validation that checks all fields are provided for architectures, if present? I could also make a case for waiting to add that handling along with update support once the feature is released and the API is fully stable!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just added a validation to check whether at least one valid arch was provided or a valid URL and SHA. Let me know if this seems good or if you were thinking of something else.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's perfect! Just a little nicer to skip making the request if we already know it's going to fail.

@natalie-todd natalie-todd force-pushed the ntodd/tf-18527/support-tool-version-architectures branch from 734e931 to ceaf38c Compare January 3, 2025 19:42
@natalie-todd natalie-todd force-pushed the ntodd/tf-18527/support-tool-version-architectures branch from ceaf38c to 56eeb74 Compare January 3, 2025 19:44
@uturunku1 uturunku1 dismissed sebasslash’s stale review January 3, 2025 21:13

Natalie addressed the changes requested by Sebastian

@natalie-todd natalie-todd merged commit 9e23f0e into main Jan 3, 2025
7 checks passed
@natalie-todd natalie-todd deleted the ntodd/tf-18527/support-tool-version-architectures branch January 3, 2025 21:15
Copy link

github-actions bot commented Jan 3, 2025

Reminder to the contributor that merged this PR: if your changes have added important functionality or fixed a relevant bug, open a follow-up PR to update CHANGELOG.md with a note on your changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants