feat: github orgs for /bake #41
Conversation
|
hey @jpmcb thank you very much for the feedback! I think I've addressed it and I added a couple more comments to what I've got so far. |
|
the review can be held off until i fix some of these linter things |
|
@jpmcb I think I have the lint issues fixed now, should be ready for a review. |
| func GetGithubRepoHTMLUrls(repos []*github.Repository) []string { | ||
| var urls []string | ||
| for _, repo := range repos { | ||
|
|
There was a problem hiding this comment.
nit: extra space here could be removed.
| return repoList | ||
| } | ||
|
|
||
| func TestFilterGithubArchivedRepos(t *testing.T) { |
| func NewGithubTokenClient(token string) *GithubAPIClient { | ||
| ctx := context.Background() | ||
| s := &GithubAPIClient{ | ||
| client: github.NewTokenClient(ctx, token), | ||
| } | ||
| return s | ||
| } |
There was a problem hiding this comment.
nit: Looks like this is dead code and isn't used. Makes sense as part of a github client package if we needed a client generated from a token in the future. Curious to hear why it was included?
| /* | ||
| Accepts URL value and calls: | ||
| normalized = common.NormalizeGitURL() | ||
| endpoint = transport.NewEndpoint(normalized) | ||
| common.IsValidGitRepo(endpoint) | ||
| return endpoint | ||
|
|
||
| returns first encountered error, appropriate HTTP error message, and endpoint string value | ||
| */ |
There was a problem hiding this comment.
nit: This comment is abit confusing. In this case I don't think we need to state what it's calling: simply summarizing that it normalizes, validates the endpoint, and gets the valid url is fine while returning the first encountered error.
| defer wg.Done() | ||
| }(htmlURL, wg, errorChannel) | ||
| } | ||
| defer wg.Wait() |
There was a problem hiding this comment.
You shouldn't defer here: this won't block on waiting for all the go routines to finish in the waitgroup and will continue on without finishing processing squashing any potential errors that might arrise in the errorChannel
| http.Error(w, "Could not process input", http.StatusInternalServerError) | ||
| return | ||
| } | ||
| } else { | ||
| go func() { | ||
| err = p.processRepository(repoURLendpoint.String()) | ||
| htmlURLs, err := p.processOrg(data.Org, data.Archives) |
There was a problem hiding this comment.
Why re-call processOrg again here? Don't you already call it to get the cloneUrls earlier?
| go func(htmlURL string, c chan error) { | ||
| _, repoURLendpoint, err := p.getValidRepoEndpoint(htmlURL) | ||
| if err != nil { | ||
| c <- err | ||
| } else { | ||
| c <- p.processRepository(repoURLendpoint) | ||
| } | ||
| }(htmlURL, errorChannel) |
There was a problem hiding this comment.
This should have a wait group or some way to block on all the go routines finishing. Otherwise, these will fire off and immediately get orphaned when the top level http request thread closes.
You'll also squash errors since you immediately go into checking the errorChannel without waiting for the work in the processRepository to potentially finish.
| p.Logger.Errorf("Could not process repository input: %v with error: %v", r.Body, err) | ||
| errorChannel := make(chan error) | ||
| wg := new(sync.WaitGroup) | ||
| wg.Add(len(cloneURLs)) |
There was a problem hiding this comment.
Hmmm I'm not sure this is the right approach.
This could cause CPU starving if we fire off a huge number of threads at once and start work in them without concern for the underlying processes. Additionally, the top level http request thread is still servicing incoming requests which may require additional threads.
I wonder if a buffered channel that we can block on is a better approach. Open to suggestions.
|
Hi @mtfoley - any progress on this? Happy to help where I can! |
|
@jpmcb Time has been fleeting lately. I've done some things with them but go routines and their trappings are definitely areas I could use some more practice with. Practical examples you know of re: buffered channels would help if you know of them. Thx for the bump! |
|
Go by example has some good stuff and a pretty light overview of the entire Go paradigm: https://gobyexample.com/channel-buffering And you can't go wrong with the standard library's documentation: https://pkg.go.dev/github.com/eapache/channels Good luck!! Please feel free to tag me in specific bits of code you are wondering about |
Description
Adds optional
organdarchivespayload params to /bake endpoint. If an org is specified in the format "https://github.com/open-sauced", the server will perform a GET request to https://api.github.com/orgs/open-sauced/repos and leverage the returned JSON to generate a list of repository URLs to process. By default, it will skip the repos that are archived, but this can be changed by settingarchives=truein POST to /bake.What type of PR is this? (check all applicable)
Related Tickets & Documents
Resolves #32
Mobile & Desktop Screenshots/Recordings
N/A
Added tests?
Added to documentation?
[optional] Are there any post-deployment tasks we need to perform?
[optional] What gif best describes this PR or how it makes you feel?