Issue #179 Refactor and test handleJenkinsUIRequest()

[kishansagathiya]

Fixes #179

[hferentschik]

So that for now is just a variable rename?

[kishansagathiya]

@hferentschik No, The code from handleJenkinsUIRequest() has been transferred to processTokenJSON() and checkCookies(). Does this refactor seem right to you? If this is the kind of refactor that you were expecting I can move to writing tests.

[hferentschik]

While working on this you might want to verify by running the Proxy locally whether it still works. There might be an issue with cookies in this case. I have not confirmed that and I successfully did a proxying using a local Proxy instance, but there is this issue which might prevent you to test the workflow locally https://github.com/fabric8-services/fabric8-jenkins-proxy/issues/174. Might be worth following up.

Being able to test the workflow locally will help a lot, but remember the end game is to "codify" the flow into some tests.

[hferentschik]

Any more changes to this?

[kishansagathiya]

Is this(https://github.com/fabric8-services/fabric8-jenkins-proxy/blob/master/internal/proxy/proxy.go#L315) the function that waits until jenkins is unidled?

[kishansagathiya]

@chmouel If this seems right to you I can move forward with tests. Except for breaking the function this doesnt have much changed.

[chmouel]

@kishansagathiya there is a few concernces about this from @hferentschik what are about?

[kishansagathiya]

@chmouel I think they are all covered

• No more naked returns
• No more Authorization Header
• redirectURL is in smaller functions
• less returned values
• conditions are out of the methods

[chmouel]

I am worried to merge something like this without any tests (i.e: no proxy_test.go) can we have unitests along this commit please?

[kishansagathiya]

@chmouel This is NOT ready for merge yet. It is WIP. We can merge this after tests. The reason for such temporary commits is to make sure that I am on the right path. If I get a review after tests, I will have change implementation and tests both.

[kishansagathiya]

@chmouel Have added some tests. It needs much more, which I keep pushing with this PR. btw do I need to mock the database as well? not sure, what should I do? fabric8-auth doesn't mock db.

[chmouel]

look at the way it uses https://github.com/ory/dockertest for the database tests, there is already some example in the in db_store_test here

[kishansagathiya]

Tested this locally to be working using

http://localhost:8080/api/info/ksagathi-preview

It unidled the Jenkins and this is on running it again.

{"component":"proxy","header":"-H Accept-Encoding: gzip, deflate, br -H Accept-Language: en-US,en;q=0.9 -H Accept: text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,image/apng,*/*;q=0.8 -H Cache-Control: max-age=0 -H Connection: keep-alive -H Cookie: JenkinsIdled=<hidden> -H Upgrade-Insecure-Requests: 1 -H User-Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/63.0.3239.132 Safari/537.36","level":"info","msg":"Handling incoming proxy request.","request":"GET /api/info/ksagathi-preview","request-hash":<hidden>,"time":"2018-04-03T16:37:41+05:30","type":"Jenkins UI"}
{"component":"proxy","level":"info","msg":"Accessing Jenkins route https://jenkins-ksagathi-preview-jenkins.b542.starter-us-east-2a.openshiftapps.com/securityRealm/commenceLogin?from=%2F","ns":"ksagathi-preview-jenkins","request-hash":<hidden>,"time":"2018-04-03T16:37:41+05:30"}
{"component":"proxy","level":"info","msg":"Found cookie <hidden>","ns":"ksagathi-preview-jenkins","request-hash":<hidden>,"time":"2018-04-03T16:37:42+05:30"}

[kishansagathiya]

So far tests are only for handleJenkinsUIRequest. We can add other tests using a different PR and a different issue.

[chmouel]

@kishansagathiya fair enough, feel free to address the mock package move and i'll be 👍 with this,

[kishansagathiya]

@chmouel on it

On Thu 5 Apr, 2018, 12:31 PM Chmouel Boudjnah, notifications@github.com wrote:

@kishansagathiya https://github.com/kishansagathiya fair enough, feel free to address the mock package move and i'll be 👍 with this,

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/fabric8-services/fabric8-jenkins-proxy/pull/198#issuecomment-378839361, or mute the thread https://github.com/notifications/unsubscribe-auth/AJttD0zCMqwTqPqg7SRJlfoLSnMKn2BTks5tlcFhgaJpZM4StVbt .

[kishansagathiya]

@chmouel Moving mocks to other package was very hairy

• It creates import cycle
• Removing import cycle would require us to move tests to a separate package say something like proxy_test, which would require us to export a lot of structs, interfaces, and methods
• I requires to move other tests also like move db_store_test to separate package. I have those changes, but thought just moving mocks to different file would be a much better solution.