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

rec api: add subtree option to the cache flush endpoint #6562

Merged
merged 4 commits into from May 10, 2018

Conversation

Projects
None yet
4 participants
@chbruyand
Member

chbruyand commented May 2, 2018

Short description

This adds support for the subtree parameter to the cache flush API endpoint.

/api/v1/servers/localhost/cache/flush?domain=example.com.&subtree=true

Fix #6550

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)

@chbruyand chbruyand added this to the rec-4.1.x milestone May 2, 2018

@rgacogne

A few remarks but it looks quite good! Love the tests!

@@ -373,10 +373,11 @@ static void apiServerCacheFlush(HttpRequest* req, HttpResponse* resp) {
throw HttpMethodNotAllowedException();
DNSName canon = apiNameToDNSName(req->getvars["domain"]);
bool subtree = (req->getvars["subtree"].compare("true") == 0);

This comment has been minimized.

@rgacogne

rgacogne May 2, 2018

Member

It might be better to check whether the subtree parameter was submitted before, by checking if req->getvars.count("subtree'') is > 0, otherwise we just create an empty string in the map, which works of course, but that doesn't seem as clean.

@@ -7,6 +7,7 @@ Cache manipulation endpoint
:query server_id: The name of the server
:query domain: The domainname to flush for
:query subtree: If set to `true`, also flush the whole subtree (default=`false`)

This comment has been minimized.

@rgacogne

rgacogne May 2, 2018

Member

Perhaps we might add a note about the version in which it will be added? I'm guessing it will be 4.1.3.

@rgacogne rgacogne added enhancement and removed feature request labels May 2, 2018

@rgacogne

This comment has been minimized.

Member

rgacogne commented May 2, 2018

One Travis failure is actually related to the new tests, so something might be wrong. I restarted the other failure which was caused by the ALIAS tests.

chbruyand added some commits May 2, 2018

@chbruyand

This comment has been minimized.

Member

chbruyand commented May 2, 2018

Thanks for your comments, I updated code and tests accordingly!

@rgacogne rgacogne requested review from pieterlexis and zeha May 4, 2018

@pieterlexis pieterlexis merged commit 8c1977c into PowerDNS:master May 10, 2018

4 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
lgtm analysis: C/C++ No alert changes
Details
lgtm analysis: JavaScript No alert changes
Details
lgtm analysis: Python No alert changes
Details

@chbruyand chbruyand deleted the chbruyand:rec-api-flush-tree branch May 10, 2018

@zeha

This comment has been minimized.

Collaborator

zeha commented May 13, 2018

Sorry, the review request got buried in other gh mail :(

rgacogne added a commit to rgacogne/pdns that referenced this pull request May 16, 2018

rgacogne added a commit that referenced this pull request May 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment