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

Add support to remove tag bindings from projects/workspaces #1023

Merged
merged 4 commits into from
Jan 6, 2025

Conversation

sebasslash
Copy link
Contributor

Description

In order to remove tag bindings using the workspace or projects API you need to serialize an empty array of bindings (as a jsonapi relationship) and pass that to the PATCH endpoint for either target resource. The problem is the current update calls for either resource sets the tag bindings field to be omitempty. So if you serialize an empty slice nothing will occur. This change adds a new method to Projects and Workspaces called DeleteTagBindings() which purposefully serializes an empty slice and sends it over the wire.

Testing plan

External links

Output from tests

Including output from tests may require access to a TFE instance. Ignore this section if you have no environment to test against.

$ TFE_ADDRESS="https://example" TFE_TOKEN="example" go test ./... -v -run TestFunctionsAffectedByChange

...

@datadog-terraform-cloud-hashicorp
Copy link

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

Datadog Report

Branch report: sebasslash/remove-tag-bindings
Commit report: 66d65ef
Test service: hashicorp/go-tfe

✅ 0 Failed, 1419 Passed, 155 Skipped, 20m 41.54s Total Time
🔻 Test Sessions change in coverage: 1 decreased (-0.2%)

🔻 Code Coverage Decreases vs Default Branch (1)

  • datadog-ci junit upload 59.3% (-0.2%) - Details

@sebasslash sebasslash force-pushed the sebasslash/remove-tag-bindings branch from d9526c2 to 9ccdce0 Compare January 6, 2025 15:48
project.go Outdated
@@ -44,6 +44,9 @@ type Projects interface {

// AddTagBindings adds or modifies the value of existing tag binding keys for a project.
AddTagBindings(ctx context.Context, projectID string, options ProjectAddTagBindingsOptions) ([]*TagBinding, error)

// DeleteTagBindings removes all existing tag bindings for a project.
DeleteTagBindings(ctx context.Context, projectID string) error
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we call this DeleteAllTagBindings just in case we ever add the ability to delete specific ones?

ctrombley
ctrombley previously approved these changes Jan 6, 2025
Copy link
Collaborator

@ctrombley ctrombley left a comment

Choose a reason for hiding this comment

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

Nice work! Smoke tested & approved with minor suggestions.

workspace.go Outdated
@@ -829,6 +832,31 @@ func (s *workspaces) AddTagBindings(ctx context.Context, workspaceID string, opt
return response.Items, err
}

// DeleteTagBindings removes all tag bindings associated with a workspace. This
// method will not remove any inherited tags.
Copy link
Collaborator

Choose a reason for hiding this comment

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

💅

Suggested change
// method will not remove any inherited tags.
// method will not remove any inherited tag bindings, which must be
// removed explicitly removed from the parent resource.

return ErrInvalidWorkspaceID
}

type aliasOpts struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

No suggestions, just curious about the intent behind the name aliasOpts? What is "alias" referring to?

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 could have probably used a more descriptive name, but normally structs that "shadow" another -- for example to override struct tag definitions -- are referred to as aliases from what I've seen.

A more descriptive name would be aliasUpdateOptions, but given this wasn't part of the public interface I decided for brevity. An alternative declaration could look like:

opts :=  struct {
		Type        string        `jsonapi:"primary,workspaces"`
		TagBindings []*TagBinding `jsonapi:"relation,tag-bindings"`
	} {
		TagBindings: []*TagBinding{},
	} 

Let me know if you would prefer this change

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's ok, no changes needed from my end.

Copy link
Collaborator

@ctrombley ctrombley left a comment

Choose a reason for hiding this comment

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

@sebasslash sebasslash merged commit 4344019 into main Jan 6, 2025
7 checks passed
@sebasslash sebasslash deleted the sebasslash/remove-tag-bindings branch January 6, 2025 18:33
Copy link

github-actions bot commented Jan 6, 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.

3 participants