Check access to GitHub repo on issuetracker initialization #70
Conversation
There was a problem hiding this comment.
@codykaup, thanks a lot for putting together this PR.
Unfortunately, it turns out to be more convoluted than what it seemed in hindsight. See my comment on the linked issue.
That's what is necessary to complete this while keeping the packages decoupled.
For example, in this PR you're making an http request in the config package, while the fetcher is the package which should wrap any kind of http calls.
If you still want to finish the PR after taking those changes into consideration, I am willing to assist in real-time if you'd want.
Otherwise, I'd suggest picking #63 which is a lot easier & doesn't require major refactoring.
Thanks for that comment, it gave me some good info to work from! I just pushed up the new layout to see what you think. Am I on the right track with what you were thinking? |
validation/validation.go
Outdated
| ) | ||
|
|
||
| // Validate checks if the user has access to the configured repo | ||
| func Validate(issueTracker config.IssueTracker, origin string) error { |
There was a problem hiding this comment.
could you move the config.Validate code in here as well?
There was a problem hiding this comment.
and also, you can follow the same approach which was started there - returning an array of errors
There was a problem hiding this comment.
I tried knocking this out and it results in quite a bit of refactoring to avoid the cyclic import between config and validation. I'm good with doing that but maybe it should be a separate PR to pull those apart? Unless I'm not thinking about it correctly which is also possible.
There was a problem hiding this comment.
Why does moving config.Validate here result in a cyclic dependency? The validation component shouldn't be depended on by any other component, other than main.go?
There was a problem hiding this comment.
I've done the extraction of the config.Validate function in this PR. Feel free to pull it after its merged
|
@preslavmihaylov sorry for the delay on this one, it's been a busy few days for me. I just pushed up some of the changes you recommended! I'm still thinking through the testing situation to remove the build flag (it feels ugly to me too). Since some testing configs include |
|
If the problem is that some testing configs include non-existent github repos, why don't you try changing them to a valid public github repo instead? |
Turns out, it was a lot easier than I thought. There were just 2 places I needed to update so this one is ready! |
validation/validation.go
Outdated
| ) | ||
|
|
||
| // Validate checks if the user has access to the configured repo | ||
| func Validate(issueTracker config.IssueTracker, origin string) error { |
There was a problem hiding this comment.
Why does moving config.Validate here result in a cyclic dependency? The validation component shouldn't be depended on by any other component, other than main.go?
validation/validation.go
Outdated
| ) | ||
|
|
||
| // Validate checks if the user has access to the configured repo | ||
| func Validate(issueTracker config.IssueTracker, origin string) error { |
There was a problem hiding this comment.
I've done the extraction of the config.Validate function in this PR. Feel free to pull it after its merged
|
The PR I mentioned previously - #73 has been merged to master. Feel free to pull & update yours. |
|
@preslavmihaylov just a heads up, this is ready for another review! Thanks for all your help on this one! |
|
@codykaup this is the final touch & it's ready to go :) |
|
Thanks @codykaup a lot for this contribution! It's officially in master 🎉 |
This PR closes #62 by making a quick request to the Github repo to ensure the user has access. If a 404 is returned, we log the output and exit early!