Skip to content
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

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

padas2
Copy link

@padas2 padas2 commented Jun 10, 2019

Initial cut of Pull Request to add support for interacting with SupportZip REST API.

Work so far done:

  • Added Code for creating and getting status of Support Zip.
  • Added Live and Mock tests for both of the above function calls.

Things yet to be done.

  • Need to implement fallbacks for SupportApi method calls.

@padas2 padas2 changed the title Feature/endpoints for support zip Add support for creation/download of Support Zip Jun 10, 2019
@padas2
Copy link
Author

padas2 commented Jun 11, 2019

@cdancy Could you please take a look at this PR and provide your inputs regarding the issue about duplication as well.

@cdancy cdancy self-assigned this Jun 11, 2019
@cdancy
Copy link
Owner

cdancy commented Jun 11, 2019

@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")
Copy link
Owner

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.

Copy link
Author

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();
Copy link
Owner

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?

Copy link
Author

@padas2 padas2 Jun 12, 2019

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)

@cdancy
Copy link
Owner

cdancy commented Jun 11, 2019

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 error objects that are on the main README of this project and every endpoint implements. Take a look at how that is done and let me know if you have any questions. Both the mock and live tests will need further tests to account for the possible failure states.

@cdancy cdancy added this to the v2.5.4 milestone Jun 11, 2019
@cdancy
Copy link
Owner

cdancy commented Jun 11, 2019

There is code duplication in 2 classes i.e. SupportZipDetails and SupportZipStatus.
Tried using the HAS-A pattern to include SupportZipDetails in SupportZipStatus class itself but looks
like it is not allowed by AutoValue.
One way to remove duplicate code would be to move components to a common class and have the 2
classes extend them, but that would violate the IS-A/HAS-A principle, so leaving that out..

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.

@cdancy
Copy link
Owner

cdancy commented Jun 11, 2019

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.

@padas2
Copy link
Author

padas2 commented Jun 12, 2019

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.

Not sure if they were meant to be similar but they sure have a lot of common components.
See below for response types in both the cases.
On sending the POST call to create a support zip, this is the response we get.

{
    "taskId": "b1481be1-e63d-48c2-bc73-c45d97261e0d",
    "progressPercentage": 0,
    "progressMessage": "",
    "warnings": [],
    "status": "IN_PROGRESS"
}

On trying to determine the status of the support Zip task id, this is the response we get.

{
    "taskId": "b1481be1-e63d-48c2-bc73-c45d97261e0d",
    "progressPercentage": 100,
    "progressMessage": "It was saved to C:\\Atlassian\\ApplicationData\\Bitbucket\\shared\\export\\Bitbucket_support_2019-06-12-11-45-43.zip.",
    "fileName": "Bitbucket_support_2019-06-12-11-45-43.zip",
    "warnings": [
        {
            "@class": "com.atlassian.troubleshooting.stp.action.DefaultMessage",
            "name": "File Truncated",
            "body": "The file atlassian-bitbucket-2019-05-19.log was truncated to 25Mb in size.  If you need to include the whole file, untick the 'Limit File Sizes' option."
        },
        {
            "@class": "com.atlassian.troubleshooting.stp.action.DefaultMessage",
            "name": "File Truncated",
            "body": "The file atlassian-bitbucket-2019-05-24.log was truncated to 25Mb in size.  If you need to include the whole file, untick the 'Limit File Sizes' option."
        },
        {
            "@class": "com.atlassian.troubleshooting.stp.action.DefaultMessage",
            "name": "File Truncated",
            "body": "The file atlassian-bitbucket-2019-05-23.log was truncated to 25Mb in size.  If you need to include the whole file, untick the 'Limit File Sizes' option."
        },
        {
            "@class": "com.atlassian.troubleshooting.stp.action.DefaultMessage",
            "name": "File Truncated",
            "body": "The file atlassian-bitbucket-2019-05-17.log was truncated to 25Mb in size.  If you need to include the whole file, untick the 'Limit File Sizes' option."
        }
    ],
    "status": "COMPLETE_WITH_WARNING"
}

@padas2
Copy link
Author

padas2 commented Jun 12, 2019

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 error objects that are on the main README of this project and every endpoint implements. Take a look at how that is done and let me know if you have any questions. Both the mock and live tests will need further tests to account for the possible failure states.

Sure.

@cdancy
Copy link
Owner

cdancy commented Jun 13, 2019

On trying to determine the status of the support Zip task id, this is the response we get.

Ok they look very similar. Lets go with creating just the one object for both endpoints and provide the @nullable annotation where necessary.

@padas2
Copy link
Author

padas2 commented Jun 13, 2019

On trying to determine the status of the support Zip task id, this is the response we get.

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.
Will resume working on this from Saturday.

@cdancy
Copy link
Owner

cdancy commented Jun 13, 2019

@padas2 sounds good!

@padas2
Copy link
Author

padas2 commented Jun 16, 2019

And @cdancy , I forgot to mention one more thing.
The REST API which we are using here to create the support zip is valid for both bitbucket server types and bitbucket data center types.
But we can send parameters as well in the REST call so as to include/exclude other files(such as thread dumps, configuration files) in the support zip.
Should that be included in this PR as well or maybe as part of another PR ?
What do you think ?

padas2 added 3 commits June 17, 2019 01:45
…calls to use only one object type, Added mock tests for both Support Api calls, Added test resources as well
@cdancy
Copy link
Owner

cdancy commented Jun 19, 2019

@padas2 would those be different endpoints or included in the response of the existing endpoints in this PR?

@cdancy
Copy link
Owner

cdancy commented Jun 19, 2019

Also ... PR looks good so far. Still need another pass through to implement ErrorsHolder on the SupportZip domain object. Take a look at the RequestStatus below for a simple case on how to implement this. This is a primary use-case for this library in that no call will fail but instead will be handed back the expected Object with some number of "errors" attached should the call fail for whatever reason. The user then can do something like:

// groovy syntax below

SupportZip supportZip = ...
if (supportZip.errors()) {
    if (supportZip.errors().last().message.equals("something bad we care about")) {
        throw new ExceptionOfSomeKind("The World is ending!!!", supportZip.errors());
    }
}

// continue as normal

I can help/guide you if need be just scream in my direction.

https://github.com/cdancy/bitbucket-rest/blob/master/src/main/java/com/cdancy/bitbucket/rest/domain/common/RequestStatus.java

@padas2
Copy link
Author

padas2 commented Jun 20, 2019

Also ... PR looks good so far. Still need another pass through to implement ErrorsHolder on the SupportZip domain object. Take a look at the RequestStatus below for a simple case on how to implement this. This is a primary use-case for this library in that no call will fail but instead will be handed back the expected Object with some number of "errors" attached should the call fail for whatever reason. The user then can do something like:

// groovy syntax below

SupportZip supportZip = ...
if (supportZip.errors()) {
    if (supportZip.errors().last().message.equals("something bad we care about")) {
        throw new ExceptionOfSomeKind("The World is ending!!!", supportZip.errors());
    }
}

// continue as normal

I can help/guide you if need be just scream in my direction.

https://github.com/cdancy/bitbucket-rest/blob/master/src/main/java/com/cdancy/bitbucket/rest/domain/common/RequestStatus.java

Was very busy for the last couple of days. Will try to get on it either today or tomorrow.

@padas2
Copy link
Author

padas2 commented Jun 20, 2019

@padas2 would those be different endpoints or included in the response of the existing endpoints in this PR?

Looks like I was mistaken.
On further verification, looks like there are different endpoints for server type and data center type.

For data center, the endpoint we gotta hit is (for generating support zips across all clusters):
http://<base-url.example.com:8080>/rest/troubleshooting/latest/support-zip/cluster

For server, the endpoint we gotta hit is:
http://<app-url.example.com:8080>/rest/troubleshooting/latest/support-zip/local

@cdancy
Copy link
Owner

cdancy commented Jun 20, 2019

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.

@padas2
Copy link
Author

padas2 commented Jun 21, 2019

Hey Chris, just a small doubt.

How and where is the ordering of any of the autovalue classes's constructor defined ?
The thing I am not clear is, why do we always have to pass the Immutable list of errors as the first parameter in any autovalue class constructor ?

Also I tried passing it in the end, but it failed with the following error

Error:(54, 41) java: incompatible types: java.lang.String cannot be converted to java.util.List<com.cdancy.bitbucket.rest.domain.common.Error>

which was fixed once I put the errors list in first place.

@cdancy
Copy link
Owner

cdancy commented Jun 21, 2019

Also I tried passing it in the end, but it failed with the following error

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);
Copy link
Owner

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());
Copy link
Owner

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();
}
}
Copy link
Owner

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.

@padas2
Copy link
Author

padas2 commented Jul 1, 2019

Hey @cdancy , so sorry I forgot to reply. But I have been too much occupied with other stuff.
Yes... I will do all the recommended changes and notify you once they are in place.

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

Successfully merging this pull request may close these issues.

2 participants