-
Notifications
You must be signed in to change notification settings - Fork 183
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add GitHub Team User Store to tools sources #6355
Conversation
How the implementation would differ if the standard GitHub token could have the permissions?
So the GH token you created and put in the KV is your personal token? And the idea is that whomever rotates it next with the secrets tool, will have their own personal token put in KV instead?
My brain's English parser throws exception on this, sorry x-D. Is Could you perhaps link to the variable group from the PR description? What is the purpose of this variable group and variable? I know they will be used in the pipeline, but how? |
...team-user-store/GitHubTeamUserStore/GitHubTeamUserStore/Constants/ProductAndTeamConstants.cs
Show resolved
Hide resolved
tools/github-team-user-store/GitHubTeamUserStore/GitHubTeamUserStore/Program.cs
Outdated
Show resolved
Hide resolved
tools/github-team-user-store/GitHubTeamUserStore/GitHubTeamUserStore/GitHubEventClient.cs
Show resolved
Hide resolved
tools/github-team-user-store/GitHubTeamUserStore/GitHubTeamUserStore/GitHubEventClient.cs
Show resolved
Hide resolved
tools/github-team-user-store/GitHubTeamUserStore/GitHubTeamUserStore/GitHubEventClient.cs
Outdated
Show resolved
Hide resolved
tools/github-team-user-store/GitHubTeamUserStore/GitHubTeamUserStore/TeamUserGenerator.cs
Outdated
Show resolved
Hide resolved
tools/github-team-user-store/GitHubTeamUserStore/GitHubTeamUserStore/TeamUserGenerator.cs
Outdated
Show resolved
Hide resolved
tools/github-team-user-store/GitHubTeamUserStore/GitHubTeamUserStore/TeamUserGenerator.cs
Outdated
Show resolved
Hide resolved
...github-team-user-store/GitHubTeamUserStore/GitHubTeamUserStore/Constants/StorageConstants.cs
Outdated
Show resolved
Hide resolved
{ | ||
internal class Program | ||
{ | ||
static async Task Main(string[] args) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is the pipeline that we will be running this tool in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is to-be-created per PR description:
I've also created the variable group, azuresdkartifacts azure-sdk-write-teams variables, which will be used in the pipeline when I create it, after this PR has been merged.
As I mentioned in another comment, would be cool if the README has a URL of the pipeline when it comes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should look at including this in the existing pipeline https://github.com/Azure/azure-sdk-tools/blob/main/eng/pipelines/pipeline-owners-extraction.yml.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@weshaggard and @konrad-jamrozik
I need to get this merged prior to creating the pipeline. Unlike other tools, this won't be being published to the NET dev feed because it's unusable outside of pipeline so there's really no point. The only test that makes sense here is to pull back what was just stored and ensure it matches the dictionary created from the team/user data and this done immediately after the call to store it. The pipeline just needs to build and run the tool to do both.
The standard GitHub Token cannot have Organization->Membership:Read permissions which means they cannot query for team membership. Furthermore, in order to query teams and membership for an org, the account that created the token needs to be a member of Azure. This is something @weshaggard and I have discussed and it was decided to use a generated token.
Yes. The secrets are in the azure-sdk-build-tools repository in the tools/secret-management/manual/AzureSDKEngKeyVault. The json files are azuresdkartifacts-azure-sdk-write-teams-github-pat.json and azuresdkartifacts-azure-sdk-write-teams-sas.json, both of which describe what needs to be done.
I don't normally link variable groups in PRs because they're public. This is the variable group that holds these two variables. Like other variables in our pipelines, these are linked to the keyvault secrets and pull their values from there which is the standard. When the values change, after a rotation, the variable group won't need to change because it'll just start pulling the new values. |
This tool simply creates a dictionary of team/users and stores them as a json string in azure blob storage. This is first part of the fix for #4487. The second part of the fix will be modifying CodeOwnersParser to get the list of users for the team and adding them to the Owners instead of throwing them out.
A large part of the why this was done this way was due to the permissions required to access the teams in GitHub. A standard GitHub token cannot have the permissions necessary to read org/team membership. It requires a fine-grained token created by someone with the organization permissions which, unfortunately, excludes bot accounts meaning it has to be from an actual user. The resulting blob string is only 44k and would only need to be pulled by the CodeOwnersParser once, if teams are encountered.
The GitHub token and blob SAS have already been added to the secret-management tool as manual secrets in the azure-sdk-build-tools repository (PR 747). I've also created the variable group, azuresdkartifacts azure-sdk-write-teams variables, which will be used in the pipeline when I create it, after this PR has been merged. Last but not least, this tool won't be published to nuget, the pipeline will build and run the tool as part of the pipeline. Also of note, because it's using a generated GitHub token, it won't affect the repository limits where it runs.