Have event processor get configs from raw githubusercontent instead of needing a sparse checkout #6030
Conversation
JimSuplizio
added
the
GitHub Event Processor
| { | ||
| try | ||
| { | ||
| HttpResponseMessage response = client.GetAsync(url).ConfigureAwait(false).GetAwaiter().GetResult(); |
There was a problem hiding this comment.
I don't know how many changes it would require in the codeowners tool, but in the future it would be nice to just make this whole function async. Not sure why it isn't.
HttpResponseMessage response = await client.GetAsync(url);
There was a problem hiding this comment.
I agree. It's most likely the fact that making this one method async would cascade through the chain of calling functions that would require things using Codeownes to be async that wouldn't normally be.
| HttpResponseMessage response = | ||
| client.GetAsync(url).ConfigureAwait(false).GetAwaiter().GetResult(); | ||
| return response.Content.ReadAsStringAsync().ConfigureAwait(false).GetAwaiter().GetResult(); | ||
| while (attempts <= maxRetries) |
There was a problem hiding this comment.
Might not be necessary in this case but there are some more standard retry patterns for HttpClient, see https://stackoverflow.com/questions/19260060/retrying-httpclient-unsuccessful-requests for examples.
| public string CreateRawGitHubURLForFile(Repository repository, string subdirectory, string fileName) | ||
| { | ||
| // https://raw.githubusercontent.com/Azure/azure-sdk-for-net/main/.github/ | ||
| // The Full URL is BaseUrl + repositoryFullName + defaultBranch + remoteFilePath + fileName |
There was a problem hiding this comment.
I thought you were planning to pull the defaultBranch out of the repository object so things like non standard default branches, like the autorest.csharp repo, would just work.
| /// <param name="repository">Octkit.Repository from the event payload</param> | ||
| public void SetConfigEntryOverrides(Repository repository) | ||
| { | ||
| CodeOwnerUtils.codeOwnersFilePathOverride = CreateRawGitHubURLForFile(repository, CodeOwnerUtils.CodeownersSubDirectory, CodeOwnerUtils.CodeownersFileName); |
There was a problem hiding this comment.
Just something to keep in mind this potentially adds more race conditions since these are global static values.
| { | ||
| // Load the config from the well known location, somewhere under the .github directory | ||
| // which is in the root of the repository | ||
| configLoc = DirectoryUtils.FindFileInRepository(RulesConfigFileName, RulesConfigSubDirectory); |
There was a problem hiding this comment.
Is it expected that folks would still use local files if they want to test new configuration files before updating? I was trying to figure out how we would validate any changes to these files in a language repo. I don't think there is any great way to validate them in the CI but it would be nice to have a way to at least run them through the tool before blindly taking changes to them in the language repo.
Instead if requiring a sparse-checkout in the action, pull the config files (CODEOWNERS and rules config) files from the raw githubusercontent.
One thing of note here: I realize it's odd that I'm using the FileHelpers from the CodeOwnersParser to load the rules config file but there's no common utilities project and I really didn't want to copy the code and have to add the retry logic in multiple places.
The other change was when rules are loaded. Previously rules were loaded when the GitHubEventClient was constructed but now, like the CODEOWNERS, which is loaded when it's first needed.
The DirectoryUtil is going to remain until we know we can absolutely remove it.