Skip to content

Conversation

philippschulte
Copy link
Member

Fix for #594.

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes to Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully run tests with your changes locally?

kpfleming
kpfleming previously approved these changes Feb 6, 2025
Copy link
Contributor

@kpfleming kpfleming left a comment

Choose a reason for hiding this comment

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

This looks fine to me, although I think we should communicate to the API owners that returning '204 No Content' when the requested IP is not present in the ACL is not terribly logical. '404 Not Found' would make a lot more sense.

@philippschulte philippschulte force-pushed the pschulte/fix-compute-acl-lookup-call branch from aa51300 to b364f6c Compare February 10, 2025 21:27
Copy link
Contributor

@kpfleming kpfleming left a comment

Choose a reason for hiding this comment

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

LGTM

@philippschulte philippschulte merged commit 29832ac into fastly:main Feb 11, 2025
3 checks passed
@philippschulte philippschulte deleted the pschulte/fix-compute-acl-lookup-call branch February 11, 2025 15:47
@@ -33,6 +34,10 @@ func Lookup(c *fastly.Client, i *LookupInput) (*ComputeACLEntry, error) {
}
defer resp.Body.Close()

if resp.StatusCode != http.StatusOK {
Copy link
Collaborator

Choose a reason for hiding this comment

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

In the case that no matching IP was found, the API will return a 204 No Content. This is not an error condition, rather a lack of results.

Instead of returning an error here, consider alternatives to allow the caller to distinguish between an error (e.g. network timeout) and this case. One idea is to return (nil, nil).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants