-
Notifications
You must be signed in to change notification settings - Fork 210
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
Implement schema methods in the HttpManagementProxy (fixes #1344) #1366
Implement schema methods in the HttpManagementProxy (fixes #1344) #1366
Conversation
No linked issues found. Please add the corresponding issues in the pull request description. |
2004098
to
ecd45cd
Compare
ecd45cd
to
bfe4a55
Compare
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.
Sorry this has taken me a little while to get to, I thought someone else was reviewing it!
Just requesting changes to add unit tests, let me know if you have any concerns.
@@ -159,12 +161,27 @@ public String getClusterName() { | |||
|
|||
@Override | |||
public List<String> getKeyspaces() { |
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.
Issue: Can we please have a unit test for this method? Maybe something that mocks apiClient.listKeyspaces()
and then just confirms that it was called?
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.
@Miles-Garnsey , is this a todo: or a suggestion: ?
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.
Sorry, an issue I think.
} catch (ApiException ae) { | ||
LOG.error("Failed to list keyspaces", ae); | ||
return Collections.emptyList(); | ||
} | ||
} | ||
|
||
@Override | ||
public Set<Table> getTablesForKeyspace(String keyspace) throws ReaperException { |
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.
Issue: Would be good to see a unit test for this too please :) Similar procedure to the other one, mock the call to the apiClient method and confirm that the processing in the rest of the method does what it should.
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.
I added both unit tests as requested.
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.
This looks like it is all passing (there is one still running but I'm confident), those tests look great too. Thanks for adding a generic method to create the mocked proxy.
I am merging this because all tests are green, it is approved, and I want the |
cd9898a
into
thelastpickle:integration/http-managementproxy
Fixes #1344
Prerequisites:
datastax-mgmtapi-client-openapi
version insrc/server/pom.xml
.