-
Notifications
You must be signed in to change notification settings - Fork 70
[CDTOOL-1197] Use bulk purge for 'purge --key' requests #1566
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
Conversation
|
Test: |
…tly/cli into rcaril/CDTOOL-1197-purge-key-to-bulk
philippschulte
left a comment
There was a problem hiding this 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.
kpfleming
left a comment
There was a problem hiding this 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.
…tly/cli into rcaril/CDTOOL-1197-purge-key-to-bulk
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:
New Feature Submissions:
Changes to Core Features:
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.