Skip to content

Ability to ignore resource instances #162

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

infinisil
Copy link

@infinisil infinisil commented Mar 29, 2025

If we want to use this project for the NixOS org (cc NixOS/org#40, @galargh), we need the ability to ignore the Nixpkgs maintainers team for now, because it has 3.5k members and changes very frequently.

While I've never touched this codebase or Typescript before, I just went ahead and spent a bit of time to make something that kind of works, but not really. I'd be happy for any input on how to make this work properly!

The problems with the current approach:

  • It requires hardcoding the instances to ignore into the code, a bit lost on figuring out how to populate that from an HCL file
  • It also requires specifying all individual members to ignore and breaks if there's unlisted members, making this currently entirely useless!
How I hackily test this locally for those who want to jump in
  • To get the right version of terraform:

    NIXPKGS_ALLOW_UNFREE=1 nix-shell -p terraform -I nixpkgs=https://github.com/NixOS/nixpkgs/archive/17f716dbf88d1c224e3a62d762de4aaea375218e.tar.gz
    
  • Get any version of nodejs:

    nix-shell -p nodejs
    
  • Create this .envrc, fill in the XXXXXXX's and run direnv allow or source .envrc if you don't have direnv:

    export TF_IN_AUTOMATION=1
    export TF_INPUT=0
    export TF_LOCK=true
    
    export TF_WORKSPACE=Infinisil-s-Test-Organization
    export GITHUB_APP_ID=1192112
    export GITHUB_APP_INSTALLATION_ID=63324326
    
    # These are the read-write keys:
    export AWS_ACCESS_KEY_ID=XXXXXX
    export AWS_SECRET_ACCESS_KEY=XXXXXXX
    export GITHUB_APP_PEM_FILE='-----BEGIN RSA PRIVATE KEY-----
    XXXXXXX
    -----END RSA PRIVATE KEY-----'
    
  • Inside the terraform directory, run

    terraform init
    terraform show -json > $TF_WORKSPACE.tfstate.json
    
  • Inside the script directory, run

  • npm ci
    

    Make changes to the code, then run this to test them:

    npm run build && npm run main
    

    Check the file github/Infinisil-s-Test-Organization.yml to see the result

  • LSP set up with https://github.com/typescript-language-server/typescript-language-server is easy and recommended, needs to be set up with your editor though

  • Can do print debugging with console.log('hello')

This "implements" ignoring of resource instances, but for my usecase of
ignoring teams:
- It hardcodes the team into the code, currently no clue how to
  integrate that into an HCL file
- It also requires specifying all individual members to ignore and
  breaks if there's unlisted members, making this currently useless..
  Not sure how to make it work for now

Reason for needing this is because the NixOS org has a frequently
changing team of ~4000 members, which we don't want to include in the
config.
Comment on lines +46 to +50
TeamMember: [
'noteam2:infinisil',
'noteam2:infinisil-github-test',
'noteam2:infinixbot'
]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given we know a TeamMember consists of the team name and the user's username, can we not just do a prefix check?

@@ -39,6 +39,16 @@ export class State {

private _ignoredProperties: Record<string, string[]> = {}
private _ignoredTypes: string[] = []
private _ignoredNames: Record<string, string[]> = {
// TODO: Populate from config
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See

this.updateIgnoredTypesFrom(`${env.TF_WORKING_DIR}/locals.tf`)
this.updateIgnoredTypesFrom(`${env.TF_WORKING_DIR}/locals_override.tf`)
and
private updateIgnoredTypesFrom(path: string) {
if (fs.existsSync(path)) {
const hcl = HCL.parseToObject(fs.readFileSync(path))?.at(0)
const types = hcl?.locals?.at(0)?.resource_types
if (types !== undefined) {
this._ignoredTypes = ResourceConstructors.map(c => c.StateType).filter(
t => !types.includes(t)
)
}
}
}
for an example of how to do this.

@winterqt
Copy link
Contributor

I made a more complete version of this at https://github.com/Infinisil-s-Test-Organization/github-as-code/tree/ignore-team-wip, which adds an ignored_teams option to locals.hcl.

My main concern with the generalized route is that it adds some complexity on the implementation's end, e.g. we have to special case Team ignores implying TeamMember ignores, and whatever else that is similar.

@galargh If the ignored_teams option in locals.tf works for you, I can clean up and submit a proper PR superseding this one.

Thanks for your work on this, it was really easy to extend. 🙂

@winterqt
Copy link
Contributor

Alternatively, we could use the general framework laid out in my patch, and add a few more common fields: think ignored_repositories implies ignoring RepositoryCollaborator and and BranchProtection.

@galargh
Copy link
Member

galargh commented Apr 7, 2025

Hey! Thank you for all the contributions <3 I think this would be a great addition to the project! Let me think about it overnight and I'll get back to you on this either tomorrow or day after.

@galargh
Copy link
Member

galargh commented Apr 13, 2025

@infinisil @winterqt I finally had a second to think it through and I think I came up with a setup that should work pretty well. I took heavy inspiration from Infinisil-s-Test-Organization/github-as-code@master...ignore-team-wip but pushed the actual process of ignoring the resources all the way to the GitHub client.

This resulted in #172 Let me know what you think. If you're OK with this approach, then I'll add some tests and we can roll it out!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants