Conversation
vbarrier
commented
Aug 5, 2017
vbarrier
commented
- Fix bug with access_token
- update tests
- update doc
Add access token support
Add access token support
add access_token support
fix error icescrum => example
bswinnerton
left a comment
bswinnerton
left a comment
There was a problem hiding this comment.
Hi @vbarrier,
It looks like there are a number of failing tests with the addition of this PR. If we can get those passing, we can give this a proper review ❤️.
|
Hi @bswinnerton Thank you for your comment. I didn't notice the errors! They have been fixed :) |
There was a problem hiding this comment.
Hi @vbarrier,
I left some comments that I'd like to hear your thoughts on.
Generally speaking, we're not accepting new features in github-services, which is what this pull request appears to be (adding support for icescrum v7). Since you've already put the work into this, I'd be okay with accepting this pull request. But please know that the preferable way to handle service-based webhooks these days is to use the normal repository / organization webhook flow, and have the icescrum service handle the things done in this pull request. This approach seems similar to what it seems Icescrum does for GitLab. The installation of these hooks for users can even be automated via this API endpoint so that all a user would have to do is click a button in Icescrum, authenticate with GitHub, and it would be configured.
docs/icescrum
Outdated
| More information on iceScrum : https://www.icescrum.com | ||
| More information on Kagilum : https://www.kagilum.com | ||
|
|
||
| Don't forget to enable Github app in iceScrum: https://www.icescrum.com/apps/github/ |
There was a problem hiding this comment.
I think we might be missing the word "the" here:
Don't forget to enable the Github app in...
docs/icescrum
Outdated
| 1. **project_key** - This is your iceScrum Project key. | ||
|
|
||
| 2. **authentication** For new iceScrum v7+ only: | ||
| 2.1 **access_token** - Access token of one team member in the project (such as the ScrumMaster). Where find it? |
There was a problem hiding this comment.
What is meant here by "Where find it?", was this intended to be a link to documentation on where to find this information?
lib/services/icescrum.rb
Outdated
| password = data['password'].to_s.gsub(/\s+/, "") | ||
| # Cloud only support access token | ||
| if data['base_url'].to_s.empty? | ||
| raise_config_error "Access token mandatory for cloud" if data['access_token'].to_s.empty? |
There was a problem hiding this comment.
Rather than having a nested trailing conditional, it might be slightly easier to read this if it were:
if data['base_url'].to_s.empty? && data['access_token'].to_s.empty?
raise_config_error "Access token mandatory for cloud"
end
lib/services/icescrum.rb
Outdated
| end | ||
|
|
||
| # Support old authentification for R6 | ||
| if data['access_token'].to_s.empty? |
There was a problem hiding this comment.
Building on my comment from above, since we already have this check here, perhaps we could consolidate them all into something like:
if data['access_token'].to_s.empty?
raise_config_error "Access token mandatory for cloud" if data['base_url'].to_s.empty?
raise_config_error "Invalid username" if data['username'].to_s.empty?
raise_config_error "Invalid password" if data['password'].to_s.empty?
endI believe this would maintain the current functionality.
| @stubs.verify_stubbed_calls | ||
| end | ||
|
|
||
| def test_push_lowcase_project_key |
There was a problem hiding this comment.
Is there value in keeping this test around to exercise https://github.com/github/github-services/pull/1219/files#diff-6b56c8a54d5e428bb9b0c89a350950d7R18?
There was a problem hiding this comment.
Are you talking about this test: "test_push_valid_custom_url ? It has been deleted already.
|
H @bswinnerton, You are right, it's not only a bug fix pull request but it is also intended to maintain our Cloud service. So I thought it was okay to also add our new (and only) access token version. I'm already working on an app that will use your API for the futur. I have also seen that we can prefill the new personal token form using a GET request. Thank you :) |
|
Hello @bswinnerton ! Any new about this pull request ready to be merged :) ? |
|
Really sorry for the late reply on this. Thanks for this PR! Currently this project is slated for deprecation and is only accepting critical bugfixes. Thank you for your time and understanding, but I will have to close this out. 💞 |
4 participants
