feature: GitHub component #2

gberenice · 2023-10-16T16:15:05Z

Info

This adds a Github component. For now, we're able to manage repositories configuration.

kevcube

I like the dynamic "list of repos" approach but then our call to the underlying module is so much of this = each.this which seems like a waste of key presses (even if copilot does all the heavy lifting)

Also conglomerating every repo into one state file is probably harmless but large blast radius.. I dunno I think it is best for us to essentially call the mineiros repo (forked with SOPS mixin) many times instead of calling it in a loop

kevcube · 2023-10-16T16:21:13Z

github/main.tf

+  source  = "mineiros-io/repository/github"
+  version = "0.18.0"
+
+  for_each = var.repos


kevcube · 2023-10-16T16:23:09Z

github/providers.tf

+provider "github" {
+  base_url = var.gh_base_url
+  owner    = var.gh_owner
+
+  token = local.gh_token_enabled ? local.gh_token : null
+
+  dynamic "app_auth" {
+    for_each = local.gh_app_auth_enabled ? ["app_auth"] : []
+    content {
+      id              = var.gh_app_auth_id
+      installation_id = var.gh_app_auth_installation_id
+      pem_file        = local.gh_app_auth_pem_file
+    }
+  }
+}


This can all be handled in environment variables and that sort of configuration will remain consistent in Spacelift, then we don't have to do the auth method selection logic in locals, but there's also merits to it being explicit in code. Especially because we don't use a chamber sort of environment variable management in local dev loop.

We rarely use env variables for this type of configuration, so I leaned towards the approach that keeps GH configuration in one place without a need to set up Spacelift stacks (in our case). With this implementation, a token can still be consumed as an env variable if not set via SOPS.
For app_auth we could work on more complex logic in the future if needed.

kevcube · 2023-10-16T16:24:21Z

github/providers.tf

+variable "gh_app_auth_id" {
+  type        = string
+  description = "The ID of the GitHub App."
+  default     = ""
+}
+
+variable "gh_app_auth_installation_id" {
+  type        = string
+  description = "The ID of the GitHub App installation"
+  default     = ""
+}
+
+variable "gh_app_auth_pem_file_secret_name" {
+  type        = string
+  description = <<-EOF
+    The name of the secret retrieved by secrets mixin that contains
+    the contents of the GitHub App private key PEM file.
+  EOF
+  default     = null
+}
+
+variable "gh_base_url" {
+  type        = string
+  description = <<-EOF
+    (Optional) This is the target GitHub base API endpoint.
+    Providing a value is a requirement when working with GitHub Enterprise.
+    It is optional to provide this value and it can also be sourced from the GITHUB_BASE_URL environment variable.
+    The value must end with a slash, for example: https://terraformtesting-ghe.westus.cloudapp.azure.com/
+  EOF
+  default     = null
+}
+
+variable "gh_owner" {
+  type        = string
+  description = <<-EOF
+    (Optional) This is the target GitHub organization or individual user account to manage.
+    For example, `torvalds` and `github` are valid owners. It is optional to provide this value
+    and it can also be sourced from the GITHUB_OWNER environment variable.
+    When not provided and a token is available, the individual user account owning the token will be used.
+    When not provided and no token is available, the provider may not function correctly.
+  EOF
+  default     = null
+}
+
+variable "gh_token_secret_name" {
+  type        = string
+  description = <<-EOF
+    The name of the secret retrieved by secrets mixin that contains the GitHub personal access token.
+  EOF
+  default     = null
+}


I prefer all variables being in variables.tf unless they're specific to a shared mixin

I was thinking of this as of future mixin, but it's not the case right now, so I've moved it to the variables.tf 👌

kevcube · 2023-10-16T16:26:52Z

github/providers.tf

+  type        = string
+  description = <<-EOF
+    The name of the secret retrieved by secrets mixin that contains the GitHub personal access token.
+  EOF


This EOF (and others) should be spaced two forward because I think now the final rendered result will be two spaces forward, b/c this line is the one with least indentation

https://developer.hashicorp.com/terraform/language/expressions/strings#indented-heredocs

Looking at it closer I might be wrong. If you have a sec to verify let me know

Yeah, Terraform analyses the lines in the sequence to find the one with the smallest number of leading spaces, and then trims that many spaces from the beginning of all of the lines.
Since there is no diff in space numbers for these vars, it renders fine. However I've updated the description format to use the expected number of spaces.

Ok I wasn't sure if it would also analyze the spaces before your EOF marker

kevcube · 2023-10-16T16:28:25Z

.gitignore

@@ -5,14 +5,16 @@
 *.tfstate
 *.tfstate.*

+# Terraform lock files
+.terraform.lock.hcl


I think we should include

The components are intended to be used as child modules, and the root module should have it's own lock file anyway. What's the reason we might want to keep it here?

I think this component is already at root module level for a consumer. The most we would do is just copy it into sts-devops, no? to wrap it an additional time and write another variable = var.variable ... block seems like too much

additionally I see no opinions online that lockfiles should not be included in child modules and don't see a reason not to.

It's just not used by Terraform, no init is run on this level.

I think this component is already at root module level for a consumer. The most we would do is just copy it into sts-devops, no? to wrap it an additional time and write another variable = var.variable ... block seems like too much

Not sure. Let's discuss on a call.

github/main.tf

Gowiem · 2023-10-16T16:55:31Z

github/main.tf

+  deploy_keys          = each.value.deploy_keys
+  deploy_keys_computed = each.value.deploy_keys_computed
+
+  # Branch Protections v3 Configuration


Can we remove V3 config so we can avoid potentially using it since it sounds like it's now legacy?

Makes sense, especially considering it may conflict with v4 branch protections if used for the same branch. Removed.

Gowiem

@gberenice I didn't check all those inputs as I imagine that's just a for_each over ALL of the vars in the child module, yeah? That seems great to me and this looks solid. One or two requests below and I'll approve when you get back to me on those!

Gowiem · 2023-10-16T22:48:58Z

@kevcube saw this comment and wanted to share thoughts:

Also conglomerating every repo into one state file is probably harmless but large blast radius.. I dunno I think it is best for us to essentially call the mineiros repo (forked with SOPS mixin) many times instead of calling it in a loop

This is a legit point of view and I could see going either way on it depending on size. Two size perspectives that are worth considering:

In our-case at Masterpoint, we have 40 something repos in this org, which I think would be be painful: We'd have a lot of separate states that would need to be managed as separate plans / applies to review / approve / deal with. I'd find that tedious if we suddenly ended up with 40 new Stacks to manage in Spacelift (even though we're not running this in Spacelift). And if we had auto-approve apply on for all of them... then I think it'd be similar concerns as if they were all in one statefile.
Our primary customer's use-case, I could see it working fine: Maybe we have ~10 separate states to manage? No big deal for now.

I'm a bit torn on it when typing it out. I think I gravitate towards the for_each pattern to help alleviate pain for issue 1, but I'm open to push back. @gberenice would definitely like your opinion.

kevcube · 2023-10-17T12:43:48Z

@Gowiem I see your point that "auto-approve vs. one mega-state file, both have issues" but I think I feel more comfortable with auto-approve. Frankly the risk with either is minimal, but if someone is making changes from local then auto-approve is irrelevant and the mega-state is still a risk.

Honestly I think auto-approve everywhere should be a goal of ours (maybe with plan outputs printed to PRs and reviewed as a part of PR)

gberenice · 2023-10-17T14:28:24Z

@gberenice I didn't check all those inputs as I imagine that's just a for_each over ALL of the vars in the child module, yeah?

Yes (god bless copilot and chatgpt).

gberenice · 2023-10-17T14:57:30Z

@Gowiem @kevcube to me, the main question is - what this component is about and how we're layering the resources.
If we think of this component as of a high-level abstraction that can be extended (for example, with terraform-github-team) then it's probably worth taking risks with megastate and avoiding pain with dealing with plenty of states.
If only repo management is expected here - then I we're choosing the path of smaller "fractions", that, in some cases, could cost us unpleasant work to manage it. Long chain of child modules is also a concern to me.

Forking the repo raises the question of sync between the projects. I bet we'll be lazy and have some changes pushed to our repo only (classic). The original repo doesn't seem to be active at the moment, but I've already noticed some minor issues worth contributing.

gberenice added 3 commits October 16, 2023 17:44

chore: adds .editorconfig + gitignore rules

7958978

feat: adds github component

837d502

chore: adds README for github

7a4c78f

gberenice requested review from Gowiem and kevcube October 16, 2023 16:15

kevcube reviewed Oct 16, 2023

View reviewed changes

Gowiem reviewed Oct 16, 2023

View reviewed changes

github/main.tf Outdated Show resolved Hide resolved

Gowiem reviewed Oct 16, 2023

View reviewed changes

gberenice added 6 commits October 17, 2023 15:52

chore: move GH provider vars to variables.tf

ae0afa7

feat: enable trunk + GHA workflow

353d208

chore: removed branch_protections_v3 + make trunk happy

632cd71

fix: grant write permisssions for checks

e874e4e

chore: formatting

94314b8

fix: grant write permisssions for checks

5ea8997

gberenice added 2 commits October 17, 2023 18:06

chore: fmt

dc903db

chore: renames the component to 'github-repositories' + updates README

d239ead

kevcube approved these changes Oct 18, 2023

View reviewed changes

Gowiem approved these changes Oct 19, 2023

View reviewed changes

gberenice merged commit d3132a4 into main Oct 20, 2023
2 checks passed

gberenice deleted the feature/gh_component branch October 20, 2023 11:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: GitHub component #2

feature: GitHub component #2

gberenice commented Oct 16, 2023

kevcube left a comment

kevcube Oct 16, 2023

kevcube Oct 16, 2023

gberenice Oct 17, 2023

kevcube Oct 16, 2023

gberenice Oct 17, 2023

kevcube Oct 16, 2023

kevcube Oct 16, 2023

gberenice Oct 17, 2023

kevcube Oct 17, 2023 •

edited

Loading

kevcube Oct 16, 2023

gberenice Oct 17, 2023

kevcube Oct 17, 2023

kevcube Oct 17, 2023

gberenice Oct 17, 2023

gberenice Oct 17, 2023 •

edited

Loading

Gowiem Oct 16, 2023

gberenice Oct 17, 2023

Gowiem left a comment

Gowiem commented Oct 16, 2023

kevcube commented Oct 17, 2023 •

edited

Loading

gberenice commented Oct 17, 2023

gberenice commented Oct 17, 2023

feature: GitHub component #2

feature: GitHub component #2

Conversation

gberenice commented Oct 16, 2023

Info

kevcube left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevcube Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gberenice Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gowiem left a comment

Choose a reason for hiding this comment

Gowiem commented Oct 16, 2023

kevcube commented Oct 17, 2023 • edited Loading

gberenice commented Oct 17, 2023

gberenice commented Oct 17, 2023

kevcube Oct 17, 2023 •

edited

Loading

gberenice Oct 17, 2023 •

edited

Loading

kevcube commented Oct 17, 2023 •

edited

Loading