-
Notifications
You must be signed in to change notification settings - Fork 57
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
Add support for creation/download of Support Zip #197
base: master
Are you sure you want to change the base?
Conversation
…i as well, Need to create Mock Test and in-line supportApi tests
…low HAS-A pattern to avoid duplication
…but looks like autovalue does not allow it, need to investigate more
@cdancy Could you please take a look at this PR and provide your inputs regarding the issue about duplication as well. |
@padas2 will do. Was out sick yesterday and planned on looking at things and providing comments end-of-day today. |
@Path("/rest/troubleshooting/latest/support-zip") | ||
@SuppressWarnings("PMD.AvoidDuplicateLiterals") | ||
public interface SupportApi { | ||
@Named("support-zip:create") |
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.
Lets put a line-break between the class name and this Named
.
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.
Done
@Consumes(MediaType.APPLICATION_JSON) | ||
@Path("/local") | ||
@POST | ||
SupportZipDetails createSupportZip(); |
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.
Can we put a Documentation
annotation here as we do elsewhere?
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.
Done though the lengths of links are very long(>= 210 chars long)
Looking things over we'll need mock-tests to cover both endpoints. Also ... both endpoints, and the objects they return, NEED to handle failure states with the |
How similar are they intended to be? If they are similar enough we can just use one object and provide the @nullable annotation when it's expected to be null. |
Also ... for downloading the zip lets leave that for a subsequent PR. I generally like to have 1 endpoint per PR to make things easier for reviewing. |
…documentation detail annotation tag
Not sure if they were meant to be similar but they sure have a lot of common components.
On trying to determine the status of the support Zip task id, this is the response we get.
|
Sure. |
Ok they look very similar. Lets go with creating just the one object for both endpoints and provide the @nullable annotation where necessary. |
So, I am going out of town for a couple of days. |
@padas2 sounds good! |
And @cdancy , I forgot to mention one more thing. |
…calls to use only one object type, Added mock tests for both Support Api calls, Added test resources as well
@padas2 would those be different endpoints or included in the response of the existing endpoints in this PR? |
Also ... PR looks good so far. Still need another pass through to implement
I can help/guide you if need be just scream in my direction. |
Was very busy for the last couple of days. Will try to get on it either today or tomorrow. |
Looks like I was mistaken. For data center, the endpoint we gotta hit is (for generating support zips across all clusters): For server, the endpoint we gotta hit is: |
Cool. Let me know if you need help and good work thus far. Wiring things up for use with jclouds, on top of the "errors" layer I've put on top, can be a bit daunting if you've never done so before. |
Hey Chris, just a small doubt. How and where is the ordering of any of the autovalue classes's constructor defined ? Also I tried passing it in the end, but it failed with the following error
which was fixed once I put the errors list in first place. |
Aaaahhhh ... that's just an AutoValue feature/quirk with how they build the generated classes. Take a look at the generated java on disk to see what they are doing. That's what I did many moons ago as the magic was too much for me to handle. |
@@ -663,6 +674,11 @@ public static PullRequest createPullRequestFromErrors(final List<Error> errors) | |||
null, null, errors); | |||
} | |||
|
|||
public static SupportZip createSupportZipFromErrors(final List<Error> errors) { | |||
return SupportZip.create(null, 0, null, | |||
null, null, null, errors); |
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 shouldn't actually work if none of the fields are annotated with @nullable.
@Test | ||
public void testSupportZipStatus() { | ||
SupportZip supportZipTask = api().createSupportZip(); | ||
supportZipTask = api().getSupportZipStatus(supportZipTask.taskId()); |
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.
Lets also test the case where things fail. Meaning we can pass in a taskId which does not exist and the returned Errors
list should be populated with objects.
} finally { | ||
mockWebServer.shutdown(); | ||
} | ||
} |
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.
We also need mock tests for when things fail on both endpoints. To trigger the failure you could use a non-existent taskId for one, combined with an error code of whatever that is supposed to return, and the other I would use an error code response of say 500
to account for insufficient privileges to do this task.
Hey @cdancy , so sorry I forgot to reply. But I have been too much occupied with other stuff. |
Initial cut of Pull Request to add support for interacting with SupportZip REST API.
Work so far done:
Things yet to be done.