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

Variable sets #305

Merged
merged 32 commits into from Mar 24, 2022
Merged

Variable sets #305

merged 32 commits into from Mar 24, 2022

Conversation

rexredinger
Copy link
Contributor

@rexredinger rexredinger commented Feb 10, 2022

Description

Fixes #284

This adds functionality for managing Variable Sets and their Variables. The plan is

Testing plan

  1. CRUD a Variable Set.
  2. CRUD some Variables against a Variable Set.
  3. Assign a Variable Set to some workspaces.

External links

Output from tests (HashiCorp employees only)

NOTE: TFE Tests are expected to fail because of extant feature flagging.

🍔_alex: go-tfe $ envchain gotfe go test -run "^TestVariableSet.+" -v ./... -tags=integration
=== RUN   TestVariableSetsList
=== RUN   TestVariableSetsList/without_list_options
    variable_set_test.go:32: paging not supported yet in API
=== RUN   TestVariableSetsList/with_list_options
    variable_set_test.go:38: paging not supported yet in API
=== RUN   TestVariableSetsList/when_Organization_name_is_invalid_ID
--- PASS: TestVariableSetsList (1.53s)
    --- SKIP: TestVariableSetsList/without_list_options (0.19s)
    --- SKIP: TestVariableSetsList/with_list_options (0.00s)
    --- PASS: TestVariableSetsList/when_Organization_name_is_invalid_ID (0.00s)
=== RUN   TestVariableSetsCreate
=== RUN   TestVariableSetsCreate/with_valid_options
=== RUN   TestVariableSetsCreate/when_options_is_missing_name
=== RUN   TestVariableSetsCreate/when_options_is_missing_global_flag
--- PASS: TestVariableSetsCreate (0.98s)
    --- PASS: TestVariableSetsCreate/with_valid_options (0.20s)
    --- PASS: TestVariableSetsCreate/when_options_is_missing_name (0.00s)
    --- PASS: TestVariableSetsCreate/when_options_is_missing_global_flag (0.00s)
=== RUN   TestVariableSetsRead
=== RUN   TestVariableSetsRead/when_the_variable_set_exists
=== RUN   TestVariableSetsRead/when_variable_set_does_not_exist
--- PASS: TestVariableSetsRead (1.27s)
    --- PASS: TestVariableSetsRead/when_the_variable_set_exists (0.10s)
    --- PASS: TestVariableSetsRead/when_variable_set_does_not_exist (0.19s)
=== RUN   TestVariableSetsUpdate
=== RUN   TestVariableSetsUpdate/when_updating_a_subset_of_values
=== RUN   TestVariableSetsUpdate/when_options_has_an_invalid_variable_set_ID
--- PASS: TestVariableSetsUpdate (0.92s)
    --- PASS: TestVariableSetsUpdate/when_updating_a_subset_of_values (0.10s)
    --- PASS: TestVariableSetsUpdate/when_options_has_an_invalid_variable_set_ID (0.00s)
=== RUN   TestVariableSetsDelete
=== RUN   TestVariableSetsDelete/with_valid_ID
=== RUN   TestVariableSetsDelete/when_ID_is_invlaid
--- PASS: TestVariableSetsDelete (0.99s)
    --- PASS: TestVariableSetsDelete/with_valid_ID (0.19s)
    --- PASS: TestVariableSetsDelete/when_ID_is_invlaid (0.00s)
=== RUN   TestVariableSetsAssign
=== RUN   TestVariableSetsAssign/with_valid_workspaces
--- PASS: TestVariableSetsAssign (1.36s)
    --- PASS: TestVariableSetsAssign/with_valid_workspaces (0.33s)
=== RUN   TestVariableSetVariablesList
=== RUN   TestVariableSetVariablesList/without_list_options
    variable_set_variable_test.go:39: paging not supported yet in API
=== RUN   TestVariableSetVariablesList/with_list_options
    variable_set_variable_test.go:45: paging not supported yet in API
=== RUN   TestVariableSetVariablesList/when_variable_set_ID_is_invalid_ID
--- PASS: TestVariableSetVariablesList (1.40s)
    --- SKIP: TestVariableSetVariablesList/without_list_options (0.17s)
    --- SKIP: TestVariableSetVariablesList/with_list_options (0.00s)
    --- PASS: TestVariableSetVariablesList/when_variable_set_ID_is_invalid_ID (0.00s)
=== RUN   TestVariableSetVariablesCreate
=== RUN   TestVariableSetVariablesCreate/with_valid_options
=== RUN   TestVariableSetVariablesCreate/when_options_has_an_empty_string_value
=== RUN   TestVariableSetVariablesCreate/when_options_has_an_empty_string_description
=== RUN   TestVariableSetVariablesCreate/when_options_has_a_too-long_description
=== RUN   TestVariableSetVariablesCreate/when_options_is_missing_value
=== RUN   TestVariableSetVariablesCreate/when_options_is_missing_key
=== RUN   TestVariableSetVariablesCreate/when_options_has_an_empty_key
=== RUN   TestVariableSetVariablesCreate/when_options_is_missing_category
=== RUN   TestVariableSetVariablesCreate/when_workspace_ID_is_invalid
--- PASS: TestVariableSetVariablesCreate (1.51s)
    --- PASS: TestVariableSetVariablesCreate/with_valid_options (0.13s)
    --- PASS: TestVariableSetVariablesCreate/when_options_has_an_empty_string_value (0.14s)
    --- PASS: TestVariableSetVariablesCreate/when_options_has_an_empty_string_description (0.11s)
    --- PASS: TestVariableSetVariablesCreate/when_options_has_a_too-long_description (0.09s)
    --- PASS: TestVariableSetVariablesCreate/when_options_is_missing_value (0.13s)
    --- PASS: TestVariableSetVariablesCreate/when_options_is_missing_key (0.00s)
    --- PASS: TestVariableSetVariablesCreate/when_options_has_an_empty_key (0.00s)
    --- PASS: TestVariableSetVariablesCreate/when_options_is_missing_category (0.00s)
    --- PASS: TestVariableSetVariablesCreate/when_workspace_ID_is_invalid (0.00s)
=== RUN   TestVariableSetVariablesRead
=== RUN   TestVariableSetVariablesRead/when_the_variable_exists
=== RUN   TestVariableSetVariablesRead/when_the_variable_does_not_exist
=== RUN   TestVariableSetVariablesRead/without_a_valid_variable_set_ID
=== RUN   TestVariableSetVariablesRead/without_a_valid_variable_ID
--- PASS: TestVariableSetVariablesRead (1.26s)
    --- PASS: TestVariableSetVariablesRead/when_the_variable_exists (0.13s)
    --- PASS: TestVariableSetVariablesRead/when_the_variable_does_not_exist (0.09s)
    --- PASS: TestVariableSetVariablesRead/without_a_valid_variable_set_ID (0.00s)
    --- PASS: TestVariableSetVariablesRead/without_a_valid_variable_ID (0.00s)
=== RUN   TestVariableSetVariablesUpdate
=== RUN   TestVariableSetVariablesUpdate/with_valid_options
=== RUN   TestVariableSetVariablesUpdate/when_updating_a_subset_of_values
=== RUN   TestVariableSetVariablesUpdate/with_sensitive_set
=== RUN   TestVariableSetVariablesUpdate/without_any_changes
=== RUN   TestVariableSetVariablesUpdate/with_invalid_variable_ID
=== RUN   TestVariableSetVariablesUpdate/with_invalid_variable_ID#01
--- PASS: TestVariableSetVariablesUpdate (1.66s)
    --- PASS: TestVariableSetVariablesUpdate/with_valid_options (0.11s)
    --- PASS: TestVariableSetVariablesUpdate/when_updating_a_subset_of_values (0.10s)
    --- PASS: TestVariableSetVariablesUpdate/with_sensitive_set (0.11s)
    --- PASS: TestVariableSetVariablesUpdate/without_any_changes (0.31s)
    --- PASS: TestVariableSetVariablesUpdate/with_invalid_variable_ID (0.00s)
    --- PASS: TestVariableSetVariablesUpdate/with_invalid_variable_ID#01 (0.00s)
=== RUN   TestVariableSetVariablesDelete
=== RUN   TestVariableSetVariablesDelete/with_valid_options
=== RUN   TestVariableSetVariablesDelete/with_non_existing_variable_ID
=== RUN   TestVariableSetVariablesDelete/with_invalid_workspace_ID
=== RUN   TestVariableSetVariablesDelete/with_invalid_variable_ID
--- PASS: TestVariableSetVariablesDelete (1.19s)
    --- PASS: TestVariableSetVariablesDelete/with_valid_options (0.13s)
    --- PASS: TestVariableSetVariablesDelete/with_non_existing_variable_ID (0.09s)
    --- PASS: TestVariableSetVariablesDelete/with_invalid_workspace_ID (0.00s)
    --- PASS: TestVariableSetVariablesDelete/with_invalid_variable_ID (0.00s)
PASS
ok  	github.com/hashicorp/go-tfe	17.820s
?   	github.com/hashicorp/go-tfe/examples/organizations	[no test files]
?   	github.com/hashicorp/go-tfe/examples/workspaces	[no test files]
?   	github.com/hashicorp/go-tfe/mocks	[no test files]

@rexredinger rexredinger requested a review from a team as a code owner February 10, 2022 19:29
Copy link
Collaborator

@brandonc brandonc left a comment

Choose a reason for hiding this comment

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

A lot of my comments were made on VariableSets but apply to VariableSetVariables as well and I didn't point them out explicitly. I noticed many requests implicitly included workspace data. Is that necessary in a way that I overlooked?

variable_set.go Outdated Show resolved Hide resolved
variable_set.go Outdated Show resolved Hide resolved
variable_set.go Outdated Show resolved Hide resolved
variable_set.go Outdated Show resolved Hide resolved
variable_set.go Outdated Show resolved Hide resolved
variable_set.go Outdated Show resolved Hide resolved
@rexredinger
Copy link
Contributor Author

I noticed many requests implicitly included workspace data. Is that necessary in a way that I overlooked?

Not required in all cases, but will be needed or useful in some workflows. I wasn't aware of the ReadOptions pattern, though. That seems better. Thanks for the review!

@rexredinger
Copy link
Contributor Author

Not sure what's causing the CI failures :(

@brandonc
Copy link
Collaborator

That is a lot of failures! Something may be up with our nightly TFE environment? I'll look into it because I'm investigating the test flakes anyway

@hashicorp-cla
Copy link

hashicorp-cla commented Mar 12, 2022

CLA assistant check
All committers have signed the CLA.

@annawinkler
Copy link
Contributor

annawinkler commented Mar 18, 2022

Is there an associated Asana issue or other documentation for this work?

I found the API page here.

@nfagerlund
Copy link
Contributor

@rexredinger (I've edited your description to add a "fixes" line, since it looks like there's an existing issue.)

Copy link
Contributor

@nfagerlund nfagerlund left a comment

Choose a reason for hiding this comment

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

Hey Alex!

  • First off: this probably needs a rebase onto main before going much further -- 1.0 happened, and some of the changes we're asking for will be easier when based on current.

  • In VariableSets.Assign(), it's not clear whether you implemented the behavior you wanted!

    We dug up two ways to modify the workspace relationship: there's PATCH on the whole varset resource while setting the relationships object, which replaces the entire set of assigned workspaces. And there's also POST on /varsets/:id/relationships/workspaces, which adds a new varset/workspace relationship without erasing the existing ones.

    The godoc comment on Assign() seems to imply that it's just assigning the varset to an additional workspace, but really it's replacing the entire set.

    If you want it to behave like the comment says, you'll need to switch the endpoint you're hitting, and you might also want to implement the DELETE /varsets/:id/relationships/workspaces endpoint in a separate Unassign() function if you're doing that.

    If you definitely want the existing behavior... maybe remove Assign entirely and document the use of Update to do the same thing? We're not sure! Alternately, you could update the comment on Assign to explain what's about to happen.

    (One thing to be aware of if you go the additive POST .../relationships route is that there's currently a nasty unhandled 500 error in the API if you try to additively modify the relationships while a workspace is set to global: true. So, potentially two API requests necessary to avoid that.)

  • The VariableSetUpdateOptions struct doesn't have omitempty on Name, so it's impossible to e.g. change a varset to global without also passing a name! Please fix that, and maybe check over the other Options structs to make sure everything that logically ought to be omitempty is.

  • Luces spotted that the error handling in this PR is now out-of-step with what the rest of the code does (part of the 1.0 stuff) -- instead of calling errors.New(), options validation errors should be referencing stuff from errors.go now.

Copy link
Contributor

@nfagerlund nfagerlund left a comment

Choose a reason for hiding this comment

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

This is still WIP, but I have a couple comments that are independent of whatever we decide about Assign(), so I'm going to go ahead and post those while the conversation is in progress!

variable_integration_test.go Outdated Show resolved Hide resolved
variable_set.go Outdated Show resolved Hide resolved
variable_set.go Outdated Show resolved Hide resolved
variable_set_variable.go Outdated Show resolved Hide resolved
variable_set.go Outdated
if !validStringID(&organization) {
return nil, ErrInvalidOrg
}
if options != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this check be moved to the valid() helper function? Like this:

if o == nil {

variable_set.go Outdated

// Update variable set to be applied to only the workspaces in the supplied list.
func (s *variableSets) Apply(ctx context.Context, variableSetID string, options *VariableSetApplyOptions) (*VariableSet, error) {
if options == nil || options.Workspaces == nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Kind of similar comment here, where we'd like to have these validations inside of options.valid(). So you'd have to add here:

if err := options.valid(); err != nil {
		return nil, err
	}

Copy link
Contributor

@nfagerlund nfagerlund left a comment

Choose a reason for hiding this comment

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

@rexredinger Ok, thank you for your patience with all the back-and-forth!

I consulted with the team about how to handle these kinds of relationship updates consistently and idiomatically, and we agreed that the current gold standard for this kind of behavior is over in Workspaces, on the RemoteStateConsumers relationship:

go-tfe/workspace.go

Lines 928 to 979 in 716b4e4

func (s *workspaces) AddRemoteStateConsumers(ctx context.Context, workspaceID string, options WorkspaceAddRemoteStateConsumersOptions) error {
if !validStringID(&workspaceID) {
return ErrInvalidWorkspaceID
}
if err := options.valid(); err != nil {
return err
}
u := fmt.Sprintf("workspaces/%s/relationships/remote-state-consumers", url.QueryEscape(workspaceID))
req, err := s.client.newRequest("POST", u, options.Workspaces)
if err != nil {
return err
}
return s.client.do(ctx, req, nil)
}
// RemoveRemoteStateConsumers removes the remote state consumers for a given workspace.
func (s *workspaces) RemoveRemoteStateConsumers(ctx context.Context, workspaceID string, options WorkspaceRemoveRemoteStateConsumersOptions) error {
if !validStringID(&workspaceID) {
return ErrInvalidWorkspaceID
}
if err := options.valid(); err != nil {
return err
}
u := fmt.Sprintf("workspaces/%s/relationships/remote-state-consumers", url.QueryEscape(workspaceID))
req, err := s.client.newRequest("DELETE", u, options.Workspaces)
if err != nil {
return err
}
return s.client.do(ctx, req, nil)
}
// UpdateRemoteStateConsumers removes the remote state consumers for a given workspace.
func (s *workspaces) UpdateRemoteStateConsumers(ctx context.Context, workspaceID string, options WorkspaceUpdateRemoteStateConsumersOptions) error {
if !validStringID(&workspaceID) {
return ErrInvalidWorkspaceID
}
if err := options.valid(); err != nil {
return err
}
u := fmt.Sprintf("workspaces/%s/relationships/remote-state-consumers", url.QueryEscape(workspaceID))
req, err := s.client.newRequest("PATCH", u, options.Workspaces)
if err != nil {
return err
}
return s.client.do(ctx, req, nil)
}

In summary, it uses three methods: UpdateRemoteStateConsumers for replacing all, and AddRemoteStateConsumers / RemoveRemoteStateConsumers for in-place modifications.

For this PR, we want to make sure that varsets are at least on track for being consistent with that approach. That means:

  • At minimum, we'd like Apply renamed to UpdateWorkspaces -- consistency with the rest of go-tfe is more important in this context than consistency with the UI for that feature. (Yes, go-tfe is already inconsistent, cf. other things like PolicySets that are almost exactly like this thing... still though!!)
  • If you can tolerate the extra work of adding the in-place add/remove functions, we would truly appreciate it! But we also know you're dealing with a lot of dependencies at the moment on this feature, so it's OK if you need to mark them as TODOs and create a follow-up ticket instead.
  • I'd like the (to-be-renamed) public VariableSetUpdateWorkspacesOptions struct to NOT include the Global field. The user can never set it and it's just an implementation detail to work around an odd interaction in the API, but it'll show up as clutter in the go-docs for that struct. I think the move is probably to have a private struct type (that includes Global) that you actually pass to client.newRequest(), and transfer the Workspaces slice from the public struct argument into that.

@rexredinger
Copy link
Contributor Author

rexredinger commented Mar 23, 2022

@uturunku1 Thanks!

@nfagerlund

Ok, thank you for your patience with all the back-and-forth!

Thanks for being patient with me 😅

  • I can and will definitely do the minimum of renaming to UpdateWorkspaces
  • I would love to jump on updating the API to support these options, but I just do not have the bandwidth at this juncture. But, rest assured, follow-up tickets will be made and followed-up on.
  • Will do!

nfagerlund
nfagerlund previously approved these changes Mar 24, 2022
Copy link
Contributor

@nfagerlund nfagerlund left a comment

Choose a reason for hiding this comment

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

Okay, I think this PR is good to go! Thank you for the revisions!

I spotted a few trivial typos in comments and in test data strings, but I'm not going to block the PR on them; you're free to merge. If you do decide to tweak them, ping me in Slack and I'll quickfast re-approve.

variable_set.go Show resolved Hide resolved
variable_set.go Outdated Show resolved Hide resolved
variable_set_test.go Outdated Show resolved Hide resolved
variable_set_test.go Outdated Show resolved Hide resolved
@nfagerlund nfagerlund dismissed brandonc’s stale review March 24, 2022 17:53

Brandon's on vacation, but I believe the change history shows all his comments got addressed!

sebasslash
sebasslash previously approved these changes Mar 24, 2022
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.

Double approve 🔥

Co-authored-by: Nick Fagerlund <nick.fagerlund@gmail.com>
@rexredinger rexredinger dismissed stale reviews from sebasslash and nfagerlund via 5bb1a6c March 24, 2022 18:00
rexredinger and others added 2 commits March 24, 2022 14:00
Co-authored-by: Nick Fagerlund <nick.fagerlund@gmail.com>
Co-authored-by: Nick Fagerlund <nick.fagerlund@gmail.com>
@rexredinger rexredinger merged commit e004de0 into main Mar 24, 2022
@rexredinger rexredinger deleted the variable_sets branch March 24, 2022 18:25
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.

Managing Variable Sets API
7 participants