Add support for GitHub Environments for Pro/Teams pricing plans

[AnitaErnszt]

The PR fixes the issue reported on #2602

In June GitHub made the environments and environment protection rules generally available, however, the wait_timer and reviewers remain limited to enterprise.

Code was written to send wait_timer and reviewers with default values if no value is present, so existing settings can be cleared.
Due to this, the code was returning 422 when called to create environments for Pro/Team private repos, as these parameters are only supported for enterprise or public repos.

PR intends to handle the error with a retry when the following conditions are met:

• error code is 422
• wait_timer is the default value (0)
• reviewers is the default value ([])

When retrying the call, a new object is sent, which only contains the parameters that are accepted for Pro/Teams private repos.

[AnitaErnszt]

@gmlewis Please kindly check if this implementation makes sense. I have tested the code locally for:

• pro/teams private repos -> successfully creates the environment
• free plan public repos -> successfully creates the environment
• free plan private repos -> 404, which is expected

private enterprise repos should have the same 'route' as the free plan public repos.

Unfortunately, I don't know how to extend the testing function to test the different 'paths'

[codecov]

Codecov Report

Merging #2611 (aa375bd) into master (c5583e7) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2611   +/-   ##
=======================================
  Coverage   97.99%   98.00%           
=======================================
  Files         127      127           
  Lines       10948    10969   +21     
=======================================
+ Hits        10729    10750   +21     
  Misses        150      150           
  Partials       69       69           

Impacted Files	Coverage Δ
github/repos_environments.go	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[gmlewis]

Thank you, @AnitaErnszt !

We definitely need to add in tests for the special cases, and it should be relatively simple.
In a new test, instead of returning a 200 OK, your could would initially return a 422, and then your test would look like all the others and return new responses.

Alternatively, and maybe this would be the best approach anyway...

Instead of making a huge indented block like we have now, you could make a new method that is NOT exported... something like createNewEnvNoEnterprise and then simply call it:

if resp != nil &&  resp.StatusCode == http.StatusUnprocessableEntity && environment != nil && len(environment.Reviewers) == 0 && environment.GetWaitTimer() == 0 {
  return s.createNewEnvNoEnterprise(u, environment)
}

Then that way, you can have two new test functions... one just to make sure the 422 triggers the if and the other test function can fully test out createNewEnvNoEnterprise (like we already do for every other existing endpoint method). Does that make sense?

[gmlewis]

I know this is in "Draft" state, but wanted to provide some feedback.
Obviously, we need two more test functions to test the added "if" condition and the new method.
Thank you!

[AnitaErnszt]

@gmlewis I have added the tests.
I wasn't sure how to decide what API response to return (422 or 200), so I used a counter for now. I'm open to suggestions

[AnitaErnszt]

On another note the contributor guide is recommending to run golint however, this project has been deprecated

[gmlewis]

Thank you, @AnitaErnszt !
LGTM.

Awaiting second LGTM+Approval before from any other contributor to this repo before merging.

[xorima]

LGTM

[gmlewis]

Thank you, @xorima !
Merging.