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

Waypoint: Require TF Project ID to be set, instead of a terraform_cloud_workspace_details resource #1052

Merged
merged 13 commits into from
Aug 21, 2024

Conversation

briancain
Copy link
Member

@briancain briancain commented Jul 17, 2024

🛠️ Description

Prior to this commit, the template and add-on definition resource required a user to set a full resource for specifying terraform_cloud_workspace_details. However, this resource required users to set a name which is a value that Waypoint does nothing with. Rather than requiring users to set this field that isn't used in the first place, this pull request instead introduces a terraform_project_id field that is required, and will use the add-on or template name for the erroneous field.

Fixes WAYP-2768 WAYP-2769

🏗️ Acceptance tests

  • Are there any feature flags that are required to use this functionality?
  • Have you added an acceptance test for the functionality being added?
  • Have you run the acceptance tests on this branch?

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccXXX'

...

@briancain briancain requested review from a team as code owners July 17, 2024 15:32
@briancain briancain force-pushed the waypoint/dont-make-details-resource-required branch from 4947ab5 to de42370 Compare July 17, 2024 23:07
@briancain briancain force-pushed the waypoint/dont-make-details-resource-required branch from 9fc16ec to 43ae8e3 Compare July 18, 2024 15:59
@briancain briancain requested a review from HenryEstberg July 18, 2024 16:00
@briancain briancain force-pushed the waypoint/dont-make-details-resource-required branch 2 times, most recently from f1a6ce9 to 5400bc3 Compare July 18, 2024 16:12
Copy link
Contributor

@HenryEstberg HenryEstberg left a comment

Choose a reason for hiding this comment

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

Looks good to me! Please make sure to run the acceptance tests and include the results in the PR description.

Copy link
Contributor

@catsby catsby left a comment

Choose a reason for hiding this comment

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

I think we should either remove/deprecate the existing terraform_cloud_workspace_details block completely, or just mark the name part of it as Optional and provide a default (like how this PR does)

@briancain briancain force-pushed the waypoint/dont-make-details-resource-required branch from f21e5ca to 0d61988 Compare August 1, 2024 15:02
Copy link
Contributor

@paladin-devops paladin-devops 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 simplifying the template's project config setting on this client's side of things.

@briancain
Copy link
Member Author

Could someone from @hashicorp/cloud-core-platform-surfaces review this PR? Thanks

@briancain briancain force-pushed the waypoint/dont-make-details-resource-required branch from f6c032c to 8ac2c1e Compare August 8, 2024 18:00
@briancain
Copy link
Member Author

There are some acceptance test failures but I think those failures are also on main and aren't related to this PR.

@briancain
Copy link
Member Author

Could one of @meirish @pierluc-codes or @jasonpilz review this? Thanks

@briancain briancain force-pushed the waypoint/dont-make-details-resource-required branch from ce39700 to 5d3fd1d Compare August 21, 2024 20:57
@briancain briancain merged commit af50703 into main Aug 21, 2024
6 checks passed
@briancain briancain deleted the waypoint/dont-make-details-resource-required branch August 21, 2024 21:37
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.

5 participants