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

Feature: add support for applying/removing variable set against workspaces #375

Merged
merged 16 commits into from May 2, 2022

Conversation

byronwolfman
Copy link
Contributor

@byronwolfman byronwolfman commented Apr 7, 2022

Description

Hello! The Terraform Cloud API has a couple of Variable Set APIs which our team would like to add for the sake of the Terraform TFE Provider which go-tfe underpins. These APIs are:

Both APIs accept a payload of the following format:

{
  "data": [
    {
      "type": "workspaces",
      "id": "ws-YwfuBJZkdai4xj9w"
    },
    {
      "type": "workspaces",
      "id": "ws-YwfuBJZkdai4xj9w"
    },
    ...
  ]
}

Testing plan

  • Added test coverage for new methods
  • Wrote some tiny programs to consume the new methods and manually verify functionality

External links

Output from tests (HashiCorp employees only)

N/A; I have access to some live orgs which I'm sure I should not be messing around with!

Copy link
Contributor

@sebasslash sebasslash left a comment

All in all, the changes look 🔥 -- There are some minor things mentioned below!

variable_set.go Outdated
@@ -235,6 +242,72 @@ func (s *variableSets) Delete(ctx context.Context, variableSetID string) error {
return s.client.do(ctx, req, nil)
}

// VariableSetApplyToWorkspacesOptions represents the options for applying variable sets to workspaces.
type VariableSetApplyToWorkspacesOptions struct {
Copy link
Contributor

@sebasslash sebasslash Apr 8, 2022

Choose a reason for hiding this comment

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

We've established a convention for v1.0 that lays out the structure for any given file: 1) Interface/Struct definitions, 2) Receiver methods and 3) validate/helper methods.

Copy link
Contributor

@sebasslash sebasslash Apr 8, 2022

Choose a reason for hiding this comment

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

Just to be clear, it should be all structs at the top of the file, followed by receiver methods and so on...

Copy link
Contributor Author

@byronwolfman byronwolfman Apr 8, 2022

Choose a reason for hiding this comment

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

Cheers, happy to move these. To be clear: would you like me to move just these newly-added structs, or all previously-existing ones too? It looks like workspace.tf follows the convention you're describing as a matter of prior art.

Copy link
Contributor

@sebasslash sebasslash Apr 8, 2022

Choose a reason for hiding this comment

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

Yes if you would not mind reorganizing the file that would be great! 🚀

Copy link
Contributor Author

@byronwolfman byronwolfman Apr 8, 2022

Choose a reason for hiding this comment

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

Addressed, but this may make the diff somewhat more difficult to read. I've split the re-arrangement into its own commit, here: ba9fb5d

variable_set_test.go Outdated Show resolved Hide resolved
sebasslash
sebasslash previously approved these changes Apr 13, 2022
Copy link
Contributor

@sebasslash sebasslash left a comment

🔥 - Ran integration test and verified the code locally

return nil, err
}
// If true the variable set is considered in all runs in the organization.
Global *bool `jsonapi:"attr,global,omitempty"`
Copy link
Collaborator

@brandonc brandonc Apr 13, 2022

Choose a reason for hiding this comment

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

This appears to be an optional value (pointer, omitempty) but we make sure the value isn't nil within VariableSetCreateOptions valid method

Copy link
Contributor Author

@byronwolfman byronwolfman Apr 13, 2022

Choose a reason for hiding this comment

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

Hey! I can't speak to this particular type. It appears in the diff because I re-ordered everything in variable_set.go at Sebastian's request, but I'm not sure I have the necessary context to understand the ask here or competently resolve it. It might make for a good GH issue to handle in a separate PR.

if options != nil {
if err := options.valid(); err != nil {
Copy link
Collaborator

@brandonc brandonc Apr 13, 2022

Choose a reason for hiding this comment

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

💅 Since nil appears to be allowed for options, you could implement this check (o == nil) within the valid() method

Copy link
Contributor Author

@byronwolfman byronwolfman Apr 13, 2022

Choose a reason for hiding this comment

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

Same as above: I suspect you're right, but I don't have any context for this, and it only appears as a diff because of this file being massively re-arranged.

wTest2, wTest2Cleanup := createWorkspace(t, client, orgTest)
defer wTest2Cleanup()

t.Run("with first workspace added", func(t *testing.T) {
Copy link
Collaborator

@brandonc brandonc Apr 13, 2022

Choose a reason for hiding this comment

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

Would love to see some test cases for invalid options

Copy link
Contributor Author

@byronwolfman byronwolfman Apr 13, 2022

Choose a reason for hiding this comment

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

Cheers, will add some more test cases!

Copy link
Contributor Author

@byronwolfman byronwolfman Apr 13, 2022

Choose a reason for hiding this comment

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

Ok, some test cases added for invalid options in bd4f215 (and hilariously corrected via 0fac7ae).

Copy link
Contributor

@sebasslash sebasslash left a comment

Fuego 🔥

@byronwolfman byronwolfman merged commit 6115330 into main May 2, 2022
4 checks passed
@byronwolfman byronwolfman deleted the byronwolfman/apply-and-remove-variable-sets branch May 2, 2022
@github-actions
Copy link

github-actions bot commented May 2, 2022

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.

None yet

3 participants