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

policyfile: add SetAndGet method #8

Merged
merged 2 commits into from
Feb 21, 2025

Conversation

oxtoacart
Copy link
Collaborator

This variant of Set returns the resulting ACL with its updated ETag.

Updates tailscale/corp#22748

@oxtoacart oxtoacart requested a review from mpminardi February 20, 2025 20:11
policyfile.go Outdated
@@ -221,6 +221,32 @@ func (pr *PolicyFileResource) Set(ctx context.Context, acl any, etag string) err
return pr.do(req, nil)
}

// Set sets the [ACL] for the tailnet and returns the resulting [ACL].
// etag is an optional value that, if supplied, will be used in the "If-Match" HTTP request header.
func (pr *PolicyFileResource) SetAndGet(ctx context.Context, acl any, etag string) (*ACL, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Should we just modify the behaviour of Set to return the *ACL instead of adding a new method, or are you thinking there is worth in keeping both here to ease v1 -> v2 migration?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added a 2nd method for three reasons:

  1. To avoid breaking clients who already use the existing method
  2. To avoid doing work that the above clients don't need (i.e. deserializing the response JSON)
  3. Set accepts both an ACL as well as a HuJSON string. For symmetry, that would imply that Set would return either an ACL or a HuJSON string depending on what was passed in, meaning that the return type would have to be any, which isn't very ergonomic.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Set accepts both an ACL as well as a HuJSON string

Having said that out loud, I'm now realizing that the new method does the same. Thinking ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Signature fixed.

@oxtoacart oxtoacart force-pushed the percy/corp22748-devices branch from 74a18f6 to 62faeb5 Compare February 21, 2025 00:12
Add 'AdvertisedRoutes', 'EnabledRoutes' and 'ClientConnectivity' to the 'Device' type
and add the method 'ListWithAllFields' to obtain Devices with those fields populated.

Updates tailscale/corp#22748

Signed-off-by: Percy Wegmann <[email protected]>
@oxtoacart oxtoacart force-pushed the percy/corp22748-policyfile branch from a6b6284 to f78cb7b Compare February 21, 2025 13:04
This variant of Set returns the resulting ACL with its updated ETag.

Updates tailscale/corp#22748

Signed-off-by: Percy Wegmann <[email protected]>
@oxtoacart oxtoacart force-pushed the percy/corp22748-policyfile branch from f78cb7b to 2b21016 Compare February 21, 2025 13:14
Copy link
Member

@mpminardi mpminardi left a comment

Choose a reason for hiding this comment

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

LGTM after a rebase / retargeting of the branch to main.

@oxtoacart oxtoacart merged commit a11ef9a into percy/corp22748-devices Feb 21, 2025
2 checks passed
@oxtoacart oxtoacart deleted the percy/corp22748-policyfile branch February 21, 2025 21:28
@oxtoacart oxtoacart restored the percy/corp22748-policyfile branch February 21, 2025 21:29
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.

2 participants