-
Notifications
You must be signed in to change notification settings - Fork 241
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
Manage membership thru IaC #1793
Manage membership thru IaC #1793
Conversation
I would guess that it does look at the current state and sync it to what ever desired state you want, isn't that how terraform is suposed to work? Maybe we can create some fake organisation to do some testing/trials and if we are satisfied with the results we can apply them on the real organisation? Looking through the groups, this may also be an opportunity to bring some structure into it, right now there are some SIGs that have teams with sub-teams, some do not have that structure, etc. Is it possible to split |
yes, although that does add an additional bit of baggage when you want to add a new one. my 0.02 is that the goal would be two files; one that is just members, and one that is SIGs/repos/whatever. members would be kinda what it is now (
|
re: 'what will happen when we run this' i actually dunno, there's gonna be subtle differences in team descriptions or whatever and the actual ID of certain things might change (not sure how the github tf provider handles this) so it's entirely possible that everyone's membership and teams will get blown away and will need to be re-added? but also maybe not? do we know anyone that's gone through a migration like this? |
We can ask via the discussion, and/or we can setup a sample github org, setup some teams & repos, etc and see what is happening. |
If it adds to much complexity, I think the added value (having SIGs in the CODEOWNER for those files) is not big enough, so 2 files sounds good to me 👍 |
I am hoping in the final version the input files can be YAML, not JSON. |
iac/membership/main.tf
Outdated
@@ -0,0 +1,23 @@ | |||
resource "github_membership" "member" { |
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.
nit: I would put this under tools/...
, e.g. tools/org_settings
, I think iac
is unnecessarily cryptic
Design question - on the original ticket someone mentioned that TF needs a state storage. Is that true? What's the approach we want to take w.r.t. such storage? I think it would be ideal if the whole thing could run on standard GH runners, not via some additional CNCF compute (which is harder to manage). A storage on local disk committed back to the repo could work (would need a different repo then). |
TF does require state storage, but there are free SaaS options we could pursue or automation to commit that state back to the GitHub repo. I would probably elect to use the SaaS option just to reduce overhead/maintenance burden. |
Regarding what would happen if we run this: my guess is that it'd fail to create teams and members because those already exist with the same names (and I believe you can't have two teams/members with the same name). Starting from scratch, Terraform state will contain nothing so there's nothing that Terraform is tracking internally. When it does the refresh at the beginning of a To import resources into the tf state (which as @austinlparker mentioned we'd preferably manage with a SaaS), we'd have to do an terraform import from the current members and teams into the new empty state. After that point, Terraform will ensure that things are kept in sync. See the import for teams for example which can work with team name or ID https://registry.terraform.io/providers/integrations/github/latest/docs/resources/team#import |
Annoyingly, there doesn't appear to be a group import so we'll need to do some sort of scripting to import everything one by one. |
So we could do import blocks (https://developer.hashicorp.com/terraform/language/import) but there's no way to loop it that I can find, so it'll be a pretty fun set of PRs.
? Given the level of pain involved here, it might be worth doing this in one bite (so putting members, teams, and repos under tf control at once). |
After thinking on it a bit and chatting with some infra folks internally, I actually think it makes more sense to more closely align the variable files with the resources. This would give us a 'members.auto.tfvars', 'repositories.auto.tfvars', 'teams.auto.tfvars', etc. It would require a little more work for people (i.e., you would need to add yourself to members and to teams) but it would make the resulting tf code much easier to maintain and reason about. Putting everything in a big yaml file is gonna require a lot of preprocessing that will be, in short, "gnarly". |
In theory the auto files go away after this initial PR though right? In the future, any new repos, teams, memberships should only be generated through terraform. I would also recommend maybe making some modules to manage these resources, that way you could define a user with their memberships in one place, not two. I wouldn't recommend import blocks, a script to go through it all is going to be your best bet. |
iac/membership/main.tf
Outdated
for_each = toset(var.members) | ||
|
||
username = each.value | ||
role = "member" // or "admin" |
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.
creating a module where the variables would be:
username = var.username
role = var.role // default role = member
team_memberships = [
{
name = "<team>",
role = "<member> // default is member
}, ...
]
Could be very helpful here to make this a bit easier on the structure/configuration.
Can Spacelift or whatever run an import script as part of state setup? We'll need to keep some sort of canonical reference (the auto.tfvars) - what would go away would be the import blocks. I think the whole end-to-end process would look something like:
|
Change to have a list of maintainers and owners, add working groups
Use a members set
Jacob and I have done a bit more work on this and I think the structure is, more or less, good... there's some things to consider, though. First, I'm not entirely sure if the membership import stuff here works correctly (honestly, I will create a dummy org to test this out on before running it against the real one). Two, we've added some abstractions around users and groups in order to support future development. It might be a little less clear, but the workfow would basically be 'add yourself to a variables file for membership, and modify the sig block in main.tf to change team roles'. |
@austinlparker Sounds good to me RE: testing on a dummy org, lmk if there are extra changes we should make. Also, it looks like we need to copy the output JSON to .auto.tfvars, right now there are no members or owners set. |
Yeah, I've been passing it in manually. Did some testing this morning in a dummy org (thanks @svrnm!) and discovered the following:
It seems like we'll need to do some imports there, working to figure those out. |
Ok, did some more testing; Team memberships can be re-created in place, but teams themselves cannot, so for each team we'll need to do something like
|
Quick update here - we need to split repo out into its own module, which I've done and not pushed yet, but I need more permissions to actually run the script. cc: @open-telemetry/technical-committee |
what's needed to push this forward? I still think implementing this will have a lot of benefits! |
maintainers = ["MadVikingGod", "pellared", "MrAlias"] | ||
} | ||
|
||
module "technical-committee_wg" { |
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 feel like there's a good reason for this, but can't we have this consistent, with either - or _ ?
source = "./modules/sig" | ||
name = "ruby" | ||
triagers = [] | ||
approvers = ["robbkidd", "ericmustin", "arielvalentin", "ahayworth", "plantfansam"] |
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.
This seems to be missing @kaylareopelle
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.
Thanks for calling this out, @jpkrohling!
I'm not really familiar with who counts as a SIG member. Should the list include everyone with a specific role on the opentelemetry-ruby repo? Should it also include folks with roles in the opentelemetry-ruby-contrib repo?
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.
This PR is very old and doesn't include changes made to members/groups over the past several months; Don't worry, people won't get missed.
|
||
module "collector-contrib-maintainer_wg" { | ||
source = "./modules/wg" | ||
name = "collector-contrib-maintainer" |
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.
What's the difference between this and the previous one?
|
||
# Replace these variables with your GitHub organization and personal access token | ||
ORG_NAME = 'open-telemetry' | ||
ACCESS_TOKEN = subprocess.run(["gh", "auth", "token"], capture_output=True).stdout.decode('utf-8').strip() |
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.
is it possible to use an env var, and if missing, call gh auth token?
import pprint | ||
|
||
# Replace these variables with your GitHub organization and personal access token | ||
ORG_NAME = 'open-telemetry' |
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 would love to be able to easily override this as well, so that I could test on my own org
# print(f"tofu import 'module.{group_name}_{suffix}.github_team_membership.sig_{singular}[\"{m}\"]' {group_name}-{member_group}:{m}") | ||
|
||
# GENERATE TF FILE | ||
with open('output.tf', 'w') as f: |
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.
Nice, I was wondering what was the origin of the members file :-) Perhaps it needs an update? Would this script be around once we make the switch?
Small nit: can we order the modules alphabetically?
@@ -0,0 +1,502 @@ | |||
|
|||
module "memberships" { |
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.
Is this a duplicate of tools/membership/main.tf?
Per #1596, I wanted to come up with a quick spike of what this would look like. This does have a slight flaw that we need to correct -- it doesn't have child teams right now. I'm also not entirely sure what'll happen when we run it...? Will it delete/re-create all the teams?
Anyway, wanted to get some code here for people to look at and bikeshed about. :)