Add integration issues API endpoint feature

[cojoj]

I've started working on the next missing official API support endpoint from our list.

This time, List the build issues produced by an integration, is on a plate 🍛.

[cojoj]

Some great amount of playing with request/response and I've prepared pretty ugly and glued from many sources but valid and showing all possibilities - JSON We'll not be able to use it to record a cassette as it's from many sources but it's a great resource to get familiar with the output.

You can find it in this gist as I don't want to paste here almost 500 lines of code... 😜

Thanks @pmkowal for helping me with this bad-boy! 👍

[pmkowal]

@cojoj always at your service :wink:

[czechboy0]

Very useful stuff guys! :+1:

[cojoj]

I've comparing those JSONs and my current conclusions are as follow:

They'll all inherit from XcodeServerEntity but eventually they have many similiar properties like:

• age
• status
• message
• type
• issueType
• integrationID
• commits

In that case I think of creating a new parent class from which specific issues will inherit... But first thing - name? XcodeServerError, GenericError?

---

Next thing is naming for individual error types (children). We've got a couple of them... buildServiceErrors and buildServiceWarnings are the same objects (have the same properties). Different class, enum? triggerErrors are special kind of errors and they have nothing in common with other objects so it's obvoius they'll be in class TriggerError errors, warnings, testFailures and analyzerWarnings are basically the same object so we're back to the story of different class for each or enum?

---

I'd appreciate some examples from your side... 😉

[czechboy0]

Yeah I'd go with Issue instead of Error. The way I see it, an Error is a type of an Issue, which could also be a Warning, for instance. I'd name the classes after this.

I agree we should just have Issue inherit from XcodeServerEntity and where needed, have specific issues inherit from Issue.

[pmkowal]

Sounds good guys.

[czechboy0]

Btw cancelling an integration when it's running results in a "Build Service Error" :laughing:

[cojoj]

Yeah, I've noticed that by:

Hmmm, let me try this scenario... Oh, f**k it, cancel, cancel, cancel! w00t, here's my buildServiceError 😄

[cojoj]

@czechboy0 I've got some mixed feelings about this whole inheritance...

Yeah I'd go with Issue instead of Error. The way I see it, an Error is a type of an Issue, which could also be a Warning, for instance. I'd name the classes after this.

Our basic class isn't an Issue (it has more paramteres than error). This is how I see this inheritance for this specific case:

                           ┌──────────────────────┐
                           │    Generic Error     │
                           └──────────────────────┘
                                       │           
                           same        │           
                    ┏━━━━━object━━━━━━━┫           
                    ┃                  │           
                    ▼                  │           
        ┌───────────────────────┐      │           
        │   BuildServiceError   │  inherits        
        └───────────────────────┘      │           
        ┌───────────────────────┐      │           
        │ BuildServiceWarining  │      │           
        └───────────────────────┘      │           
                                       │           
                                       │           
        ┌───────────────────────┐      │           
        │     TriggerError      │◀─────┤           
        └───────────────────────┘      │           
                                       │           
                                       │           
        ┌───────────────────────┐      │           
     ┌──│         Issue         │◀─────┘           
     │  └───────────────────────┘                  
 inherits                                          
     │       ┌──────────────────┐                  
     ├──────▶│      Error       │                  
     │       └──────────────────┘                  
     │       ┌──────────────────┐                  
     ├──────▶│     Warning      │                  
     │       └──────────────────┘                  
     │       ┌──────────────────┐                  
     ├──────▶│   TestFailure    │                  
     │       └──────────────────┘                  
     │       ┌──────────────────┐                  
     └──────▶│ AnalyzerWarning  │                  
             └──────────────────┘                  

And as I've mentioned before:

buildServiceErrors and buildServiceWarnings are the same objects (have the same properties). Different class, enum? triggerErrors are special kind of errors and they have nothing in common with other objects so it's obvoius they'll be in class TriggerError errors, warnings, testFailures and analyzerWarnings are basically the same object so we're back to the story of different class for each or enum?

Number of properties each class will have is presented on the graph above (top-down), so BuildServiceError will only implement basic properties and AnalyzerWarnings (and equals) will have more properties... Do you really think that subclassing from class with more properties is the proper approach? In that scenario we'll ignore some of properties what is weird... Also, I think we don't need classes like Error, Warning, TestFailure or AnalyzerWarning (basically those inheriting from Issue) as in the main object they'll be presented as [Issue]. TriggerError will have to stay TriggerError. And buildServiceErrors and buildServiceWarnings will become [GenericError].

[czechboy0]

Hmm this is unnecessarily painful. You know what? Let's simplify this.

Create an enum with all these "error" types, create one Issue class, which will have

• this enum value to be able to tell what type of an issue it was
• payload dictionary with its raw contents.

If someone cares about the nitty gritty details, they can pull it out themselves. I feel like what we're doing might be beyond the scope of an API wrapper.

[cojoj]

Just to check we're on the same track... Do you want to create something like:

class Issue: XcodeServerEntity {

    enum IssueType {
        case Warning
        case Error
        ...
    }

    let type: IssueType
    let payload: [String: AnyObject]

}

I think we can extract similar values to properties (we already have Commits and providing easy access to message would be nice...). Similar properties are as follow:

• age
• status
• message
• type
• issueType
• integrationID
• commits

---

My proposal is to create something like:

class Issue: XcodeServerEntity {

    enum IssueType {
        case Warning
        case Error
        ...
    }

    let payload: [String: AnyObject]

    let messge: String?
    let type: IssueType
    let issueType: String
    let commits: [Commit]
    let integrationID: String
    let age: Int
    let status: IssueStatus

}

What do you think?

[czechboy0]

A bit of a hybrid. Ok, only put the ones that are present in all variants of the Issue type, nothing specific to any of them. :+1:

On Tue, Aug 4, 2015 at 8:49 AM Mateusz Zając notifications@github.com wrote:

Just to check we're on the same track... Do you want to create something like:

class Issue: XcodeServerEntity {

enum IssueType {
    case Warning
    case Error
    ...
}

let type: IssueType
let payload: [String: AnyObject]

}

I think we can extract similar values to properties (we already have Commits and providing easy access to message would be nice...). Similar properties are as follow:

• age
• status
• message
• type
• issueType
• integrationID
• commits

---

My proposal is to create something like:

class Issue: XcodeServerEntity {

enum IssueType {
    case Warning
    case Error
    ...
}

let payload: [String: AnyObject]

let messge: String?
let type: IssueType
let issueType: String
let commits: [Commit]
let integrationID: String
let age: Int
let status: IssueStatus

}

What do you think?

— Reply to this email directly or view it on GitHub https://github.com/czechboy0/XcodeServerSDK/pull/93#issuecomment-127509866 .

[cojoj]

Yeah, I know... Those 7 properties I've mentioned are similar to all Issues (No matter if its buildServiceError or Warning).

[czechboy0]

Then I'd say go ahead with it. On Tue, Aug 4, 2015 at 9:32 AM Mateusz Zając notifications@github.com wrote:

Yeah, I know... Those 7 properties I've mentioned are similar to all Issues (No matter if its buildServiceError or Warning).

— Reply to this email directly or view it on GitHub https://github.com/czechboy0/XcodeServerSDK/pull/93#issuecomment-127526565 .

[cojoj]

@czechboy0 if you get any sec, please do me a favour and review this prototype of Issue class. In at a time shortage during last couple of days and that's the result - commiting something not even finished... 😞 Please be understanding 🙏 I promise I'll be a better contributor after I get back from vacation 👯

[cojoj]

I've finished writing init for Issue... Cross fingers it'll work! I'll add some tests tomorrow to check if this is actually working.

---

To make sure - do we propagate handle every exception like a good person or cut this bastard off and crash it? 😜 Just thinking what approach should I take for Optional Type(rawValue: json.stringForKey("type")). Right now I'm ❗ing it as I assume we won't get other things from JSON...

[czechboy0]

Yeah I'd prefer to change the parsing initializer to a failable one and just return nil if we can't find the required keys. For now just ! it, yeah :-/

[czechboy0]

@cojoj Great stuff, thanks! And please enjoy your holiday, your PR still will be here when you come back :wink:

[buildasaur]

Result of Integration 1

Duration: 1 minute and 37 seconds Result: Perfect build! All 65 tests passed. :+1: Test Coverage: 55%.

[buildasaur]

Result of Integration 2

Duration: 1 minute and 27 seconds Result: Perfect build! All 68 tests passed. :+1: Test Coverage: 56%.

[cojoj]

I've fixed issues reported by @czechboy0 Also, test cases were added to Issue and guess what... They're all passing 💚

Great success!

[buildasaur]

Result of Integration 3

Duration: 57 seconds Result: Perfect build! All 68 tests passed. :+1: Test Coverage: 56%.

[cojoj]

Updated Type enum with case BuildServiceWarning = "buildServiceWarning". Maybe this was caused by differences in XCS versions API?

[czechboy0]

I'm on the latest OS X Server 5 and Xcode 7.

[cojoj]

I guess there's no harm in this case for either API version 😉

[czechboy0]

One last comment (I'd suggest you remove your email address from there). Looks great, just please edit the readme to accommodate this new API support? :+1:

[cojoj]

... This is not complete, yet 😆 I guess it's a half way. Now need to implement IntegrationIssues class and write tests for it.

[buildasaur]

Result of Integration 4

Duration: 1 minute and 1 second Result: Perfect build! All 68 tests passed. :+1: Test Coverage: 56%.

[cojoj]

@czechboy0 looks like we have some issues with new Xcode 7b5... Already created PR in BuildUtils and DVR. If you can take a look, merger, change or whatever and update Podspec so I don't have to manually change everything in Pods/ 😬

[buildasaur]

Result of Integration 5

Duration: 56 seconds Result: Perfect build! All 68 tests passed. :+1: Test Coverage: 56%.

[cojoj]

@czechboy0 https://github.com/venmo/DVR/pull/18 has been merged. I suggest you pull changes to your fork so we can keep DVR up-to-date 😉

[czechboy0]

I think we're ready here, pull from swift-2 into this branch.

[cojoj]

New Xcode was complaining... That's why I've moved it up. I couldn't call it from test cases...

[czechboy0]

What was it complaining about?

[cojoj]

Don't remeber exactly (don't have access to Xcode right now) but as far as I remeber it was something like: Can't find Issue.Type.Type.BuildServiceError...

Listen, I probably won't have time today (leaving for 🇭🇷) but I'll look into this ASAP (tomorrow afternoon)... If you want to support β5 you can merge this PR (works and won't break anything) and I'll still work on this branch.

[czechboy0]

No rush, I actually fixed Beta 5 in swift-2 branch already (and released a quick update to the spec), so we are Beta 5 compatible. Let's take our time on this PR to do it properly, no need to merge this under pressure :)

[buildasaur]

Result of Integration 6

Duration: 1 minute and 12 seconds Result: Perfect build! All 68 tests passed. :+1: Test Coverage: 56%.

[cojoj]

@czechboy0 regarding keeping Type internal to Issue I get this in test cases: Maybe you've got any idea why this happens...?

[czechboy0]

Yeah Issue is a class in both BuildaGitServer (GitHub) and in XcodeServerSDK. Just remove import BuildaGitServer from the purely XcodeServerSDK classes, so that it's not ambiguous.

[cojoj]

But I can't find any BuildGitServer... Even Xcode can't find anything like this in whole XcodeServerSDK! 😕

[czechboy0]

Oh right, that was in Buildasaur. Hmm, still try to prepend Issue with XcodeServerSDK to fully qualify the name. And try to clean and the whole dance. Does that help?

[cojoj]

The whole dance thing didn't help... Got to rename this to IssueType 😉

[czechboy0]

Close enough :laughing:

[cojoj]

One thing left - test cases. Please check if this structure suits you @czechboy0 😉

---

Issue class is well tested so I guess there's no need to configure XCS to handle turbo-mode IntegrationIssues, right? Just to check if it can handle a lot Issues in one JSON 📦

[buildasaur]

Result of Integration 7

Duration: 1 minute and 10 seconds Result: Perfect build! All 68 tests passed. :+1: Test Coverage: 56%.

[czechboy0]

Yup. Looks good.

Sent from my iPhone

On Aug 14, 2015, at 9:24 AM, Mateusz Zając notifications@github.com wrote:

One thing left - test cases. Please check if this structure suits you @czechboy0 😉

— Reply to this email directly or view it on GitHub.

[cojoj]

I've added a 📼 for IntegrationIssues. I've tried to genrate some nice errors. I still need to remove some private data from this JSON...

Anyway, test case is available, passing but still missing some cool assertions. I'll try to write them tomorrow and probably finish this PR.

---

@czechboy0 please take a 👀, especially at this:

self.errors = json.dictionaryForKey("errors").allValues.filter { $0 is NSDictionary }.map { Issue(json: $0 as! NSDictionary) }

Maybe we can introduce easier way as this code looks like 🍝

[cojoj]

It looks like I've finally managed to finish this Pull Request... 😓

[czechboy0]

Thanks @cojoj, I'll take a look tomorrow!

[czechboy0]

Btw are these cassettes recorded with the latest OS X Server & Xcode betas?