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

Refactor Proxy.handleJenkinsUIRequest #179

Open
hferentschik opened this issue Feb 27, 2018 · 4 comments
Open

Refactor Proxy.handleJenkinsUIRequest #179

hferentschik opened this issue Feb 27, 2018 · 4 comments

Comments

@hferentschik
Copy link
Contributor

  • Split method into smaller parts
  • Untangle the mixture of returning values (needsAuth, noProxy) as well as writing to the response writer on the fly
  • Add unit tests
@hferentschik
Copy link
Contributor Author

For this issue, it is essential to add unit tests, if possible prior refactoring the code. This method contains some of the core Proxy logic. I want to get away from relying on manual testing.

I would even go so far as to say, making this testable in a proper way is the harder problem than the actual refactoring.

@kishansagathiya I think it makes sense to outline the approach you want to take first, prior to getting too deep into changing code.

@kishansagathiya
Copy link
Member

@hferentschik
Split handleJenkinsUIRequest into two more methods to be used inside it as helpers

  • processTokenJSON: processes token_json if present in a query and find user info and login to Jenkins
  • checkCookies: checks cookies and proxy cache to find user info

Tests:
Come up with various test scenarios, One being successful test scenario, an occurrence of each error would be a scenario and likewise. For tests depending on the scenario check whether the appropriate value of ProxyCacheItem is returned and verify that logs are as expected. Check values of variables like noProxy and needsAuth.
If cache is set by processTokenJSON, check that cache information can be retrieved using checkCookies

@hferentschik
Copy link
Contributor Author

The key is to also understand what's happening in the live system. Are you sure you have an understanding yet what this code does? Aka, token, cookies, etc?

@kishansagathiya
Copy link
Member

From what I understand,
You have two parts in handleJenkinsUIRequest
in first part

  1. check if a token_json is passed through query to the url. Extract osioToken/ Auth service token from token_json and you get ProxyCacheItem (which contains also necessary info like namespace, scheme, route, clusterURL etc) using token_json.
  2. then if jenkins is idle you set ProxyCacheItem as cookie
  3. Then using osioToken it gets Openshift online token
  4. then it logs in to Jenksins
  5. If the login is successful then set the Session cookie

I can write a note about second part which checks cookies.
But is this what you are expecting?
Also if you think this might be too complicated for me, feel free to suggest a different issue. I just picked it because I had nothing to work on :)

chmouel pushed a commit that referenced this issue Apr 9, 2018
	- Splitted handleJenkinsUIRequest() to smaller methods
	- Added error package, removed naked returns, decreased returned parameters
	- Added tests
chmouel added a commit that referenced this issue Apr 10, 2018
@sthaha sthaha assigned sthaha and unassigned kishansagathiya May 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants