add global list record sets endpoint #85
Conversation
Makefile
Outdated
| @@ -8,7 +8,7 @@ IMG=${DOCKER_NAME}:${VERSION} | |||
| LATEST=${DOCKER_NAME}:latest | |||
| BATS=github.com/sstephenson/bats | |||
| VINYLDNS_REPO=github.com/vinyldns/vinyldns | |||
| VINYLDNS_VERSION=0.9.3 | |||
| VINYLDNS_VERSION=0.9.6 | |||
There was a problem hiding this comment.
doesn't matter a whole lot, but since we just released 0.9.7 probably should go there
There was a problem hiding this comment.
Got it. Updated and tested with 0.9.7.
|
Thanks @jagadeesh545 ! Will see if I can round up the reviews @mdb |
| @@ -1 +1 @@ | |||
| [] | |||
| "id":"global-acl-group-id","name":"globalACLGroup","email":"email","status":"Active" | |||
There was a problem hiding this comment.
note: i am loosely familiar with this code; however, this file says groups_non_json but you seem to have added a group. Perhaps there is a different figure file that should be used instead?
Same above
There was a problem hiding this comment.
@jagadeesh545 Yes; Could you please clarify why the groups fixtures and tests have been modified?
There was a problem hiding this comment.
There seems to be a default globalACLGroup as part of the local vinyldns docker instance that gets launched as part of testing. Hence, I had to update the fixture to account for this default group.
There was a problem hiding this comment.
Ah, thanks for clarifying and apologies for the confusion. This is likely a side effect of updating to VinylDNS 0.9.7. Perhaps @pauljamescleary can offer some insight?
| fixture="$(cat tests/fixtures/groups_none)" | ||
|
|
||
| [ "${output}" = "${fixture}" ] | ||
| $ew groups | grep "${fixture}" |
There was a problem hiding this comment.
@jagadeesh545 Just a nitpick: Is there a reason this change -- which impacts the groups tests -- appears in a pull request pertaining to the addition of a search-record-sets command? I just want to be sure I understand :)
There was a problem hiding this comment.
After updating the fixture to accommodate the default group globalACLGroup, I also had to update to the test case to look for text present in the fixture instead of checking for an exact match.
| @@ -1 +1 @@ | |||
| [] | |||
| "id":"global-acl-group-id","name":"globalACLGroup","email":"email","status":"Active" | |||
There was a problem hiding this comment.
@jagadeesh545 Yes; Could you please clarify why the groups fixtures and tests have been modified?
tests/vinyldns_tests.bats
Outdated
| for record_set in ${record_sets} | ||
| do | ||
| [[ "${output}" =~ "${record_set}" ]] | ||
| done |
There was a problem hiding this comment.
This looks reasonable, though I'm curious to hear a bit more about why you chose to diverge from the established pattern in the other tests where a "fixture" is used.
There was a problem hiding this comment.
At the time of writing this test case, I went with a simple way to test. Now the test case is updated to match the established pattern.
tests/vinyldns_tests.bats
Outdated
| @@ -166,6 +159,21 @@ load test_helper | |||
| [ "${output}" = "${fixture}" ] | |||
| } | |||
|
|
|||
| @test "search-record-sets" { | |||
There was a problem hiding this comment.
Do you think it's worth adding something like the following, so this is consistent with the style elsewhere?
| @test "search-record-sets" { | |
| @test "search-record-sets (when the search returns results)" { |
There was a problem hiding this comment.
Added suggested context.
Requirements
Description of the Change
The VinylDNS API has been updated to include a new global recordset search endpoint at
/recordsets.This pull request includes changes required to add the new endpoint as a command to
vinyldns-cli. A new commandsearch-record-setsis added to the list of existing commands. This new command requiresrecord-name-filterargument which is a search string with at least two alpha-numeric characters. Other optional arguments includestart-from,max-items,record-type-filter,record-group-owner, andname-sort.The command uses a new method
searchRecordSets. This method extracts the given arguments and passes them to theRecordSetsGlobalListAllmethod, which in turn invokes the/recordsetsendpoint. The results will be formatted and returned.Why Should This Be In The Package?
This change should be included in the package in order to support the newly created global recordset search endpoint
/recordsets.Benefits
This change will allow users to search for recordsets across all zones.
Possible Drawbacks
Since this is a global search it might be resource-intensive. Hence, should be used cautiously.
Verification Process
A new regression test is added in
vinyldns_tests.batto validate the newly added commandsearch-record-sets. The version of vinyldns is updated from0.9.3to0.9.6as the/recordsetsendpoint is missing in0.9.3version.This caused a couple of test cases to fail. Those test cases are also updated to match vinyldns version
0.9.6.Applicable Issues (Optional)
Fixes #82