-
Notifications
You must be signed in to change notification settings - Fork 69
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
Team Management Automation Enhancement: Add team linter workflow and linter script #208
base: main
Are you sure you want to change the base?
Conversation
…at involve the teams directory Signed-off-by: Jef Spaleta <[email protected]>
I've implemented this in the MillionDollarIdeas org to fire on PR changes as a test |
- name: check team membership for org membership | ||
run: tools/lint_members.sh ladder/teams cilium | ||
env: | ||
GITHUB_TOKEN: ${{ secrets.TM_ORG_READ_TOKEN }} |
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.
Note (for me): Set up this token.
tools/lint_members.sh
Outdated
-H "Accept: application/vnd.github+json" \ | ||
-H "Authorization: Bearer $GITHUB_TOKEN" \ | ||
-H "X-GitHub-Api-Version: 2022-11-28" \ | ||
https://api.github.com/orgs/$2/members | jq -r '.[].login'` ) |
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.
May be worth checking whether this list is truncated.. I suspect that there's pagination going on, given the number of org members.
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.
updated in new commit
tools/lint_members.sh
Outdated
-H "Accept: application/vnd.github+json" \ | ||
-H "Authorization: Bearer $GITHUB_TOKEN" \ | ||
-H "X-GitHub-Api-Version: 2022-11-28" \ | ||
https://api.github.com/orgs/$2/members | jq -r '.[].login'` ) |
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.
May be worth checking whether this list is truncated.. I suspect that there's pagination going on, given the number of org members.
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.
updated in commit
- uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1 | ||
# Lint team membership files for org members | ||
- name: check team membership for org membership | ||
run: tools/lint_members.sh ladder/teams cilium |
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.
For the recent additions to the org, I have merged these files first and then run a sync script that detects such member cases and adds them into GitHub. This linter reverses this, meaning that the member would have to be in the GitHub org first before we sync. Will need new scripting to work this way.
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.
That said I think there's multiple ways to solve this, but having a linter like this in this repo seems like the right approach. Maybe I'll just rethink the sync script or rip it out and go back to the old process for adding new members.
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 look at it this way this linter is a reminder to run the sync. Worst case is you rekick the failed workflow after the sync and that should clear the PR for merge.
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.
but the sync could run against the PR checkout right?
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 guess I could rebuild the linter logic just to check for casing errors but wrong/missing org login seems like a good thing to catch.
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.
yep sync could be run against the PR checkout.
Signed-off-by: Jef Spaleta <[email protected]>
@joestringer |
Which permissions are necessary for the token? Could we use automatic token authentication? In other words, are the required permissions listed here? |
@joestringer sadly its Org scoped permissions not Repo scoped so I dont think the automatic perms work. Needed perms are organizational member read access. I'm attaching a screenshot from the MillionDollarIdeas personal access token settings I'm using to test the script logic currently. |
OK. Have you tried creating an "external contribution" from a different repository onto the Million Dollar Ideas repo to test this? What I'm wondering is, if an external contributor creates a PR against this repository, will the workflow be able to successfully access the secret in order to perform this linting step? I ask because of this workflow using |
let me try to create a throw away github account and do a test on MDI.. hold please |
Okay this is really weird The linter isn't firing at all in MDI org now, but it definitely fired last week as seen in the actions log for the MDI repo. Need to sort out why this isn't firing on pull_request there at all. |
okay sorted it out on MDI. @joestringer I think you are correct. Pull request from forked repo branch gets 401 error: |
@jspaleta hmm yeah that was my concern. I think whatever linting we do in public, ideally that can just work for any submitted PR. If we could verify that the specified usernames exist in the generic GitHub API without any privileges into the Cilium organization then maybe that would be sufficient for that particular check? |
@joestringer
both urls return info, but you can't actually use the camelcase programmatically in the post data to update teams. Its the very fact that github api urls are case-insensitive but the actual usernames are case sensitive when used as API data payloads that actually causes the need to lint. I'm gonna need to parse the json output of the get username url for the 'login' field which is the correct case sensitive data. I'll poke once I refactor a new version and commit. |
This enhances team-management automation by adding a linter workflow to be run on pull requests that involve the ladder/teams directory
The linter script uses a GitHub API token provisioned with Cilium Organization membership read access to discover all current GitHub Cilium Org logins and will compare logins listed in the team management yaml files located in ladder/teams and will fail if the team management files reference a GitHub login that is not already a member of the Cilium GitHub org. This will also lint for simple human errors such as login casing (GitHub logins are case sensitive)
Note 1:
The workflow included here is inoperable until there is a correctly named/provisioned token secret with read-only access to cilium org membership in the team-management environment. I'll need to sync with the Cilium org admins to get that provisioned as part of merging this PR.
Note 2:
Adding new logins to the GitHub Cilium Org is still beyond the scope of the team management automation and requires the manual action of GitHub Cilium Org admins, but once a GitHub login is added as an Org member, they can be added/removed to teams via file edits to files under ladder/teams. This linter should be enough to alert to situations where a new team members needs to be first added to the cilium org.
Note 3:
I've tested a version of the linter script in the MillionDollarIdeas.
You can see example failure output here:
https://github.com/MillionDollarIdeas/team-assignments/actions/runs/13151412034/job/36699409589