Skip to content

Conversation

@rcaril
Copy link
Contributor

@rcaril rcaril commented Oct 27, 2025

Change summary

The PR modifies the 'purge --key' command to use the bulk purge API behind the scenes. This was a necessary change due to purge requests with spaces and other characters being serialized due to the nature of the POST /service/{service_id}/purge/{surrogate_key} endpoint.

Internal discussion on this can be found here:
https://fastly.slack.com/archives/C01E7FV8P5H/p1761241147473289

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?

New Feature Submissions:

  • Does your submission pass tests?

Changes to Core Features:

  • Have you written new tests for your core changes, as applicable?
  • Have you successfully run tests with your changes locally?

User Impact

This change will not apply any noticeable behavior to the end user. The commands responses will be identical.

Are there any considerations that need to be addressed for release?

Purge test file has been update to use variables instead of hard coding all test items.

@rcaril rcaril marked this pull request as ready for review October 27, 2025 19:51
@rcaril rcaril requested a review from a team as a code owner October 27, 2025 19:51
@rcaril rcaril requested a review from philippschulte October 27, 2025 19:51
@rcaril
Copy link
Contributor Author

rcaril commented Oct 27, 2025

Test:

 make test TEST_ARGS="-run TestPurgeKey ./pkg/commands/purge" ok github.com/fastly/cli/pkg/commands/purge 1.064s 
@rcaril rcaril requested a review from cxreg October 28, 2025 17:56
Copy link
Member

@philippschulte philippschulte left a comment

Choose a reason for hiding this comment

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

Thanks for the fix — I appreciate the switch to the bulk purge endpoint to address key serialization issues. This is definitely the right approach for problematic keys (e.g., keys with spaces or slashes).

However, I’m concerned about one trade-off introduced here: since the bulk purge endpoint doesn’t return a status, we’re now hardcoding "Status: ok" in the CLI output. While this maintains compatibility with prior output, it may mislead users into thinking a purge succeeded when the key wasn’t actually purged.

I’m wondering if we could take a more dynamic approach here:

Detect whether the key is "safe" for the single purge endpoint by checking if key == url.PathEscape(key), and if so, use the original single purge API (/purge/) which returns real status.

Fall back to the bulk purge endpoint for keys that would be altered by encoding.

This would preserve the benefits of the existing implementation while retaining meaningful status for keys that don’t require escaping.

That said, I realize this adds complexity and potentially inconsistent behavior across keys, so I’m deferring the final decision to @kpfleming . If @kpfleming agrees that hardcoding Status: ok is acceptable then it should be clearly documented as assumed, not returned in the CHANGELOG.

@rcaril rcaril requested a review from kpfleming October 31, 2025 13:37
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.

Some small comments, otherwise the code looks good.

@rcaril rcaril merged commit 1f9346a into main Oct 31, 2025
13 checks passed
@rcaril rcaril deleted the rcaril/CDTOOL-1197-purge-key-to-bulk branch October 31, 2025 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants