GCSS-1135: Add support for GitHub environments with deployment policies by ljuboops257 · Pull Request #28 · G-Research/github-terraformer

ljuboops257 · 2025-11-18T20:22:03Z

Summary

Adds support for managing GitHub repository environments with deployment policies and reviewers, implemented as a feature-flagged capability in both the importer and Terraform provisioning layer.

What changed

Importer (`feature/github-repo-importer/`)

Imports existing GitHub environments (protection rules, reviewers, deployment policies) when features.github_environments: true is set in import-config.yaml
Feature flag format: nested features: block (not a flat key)

# config/import-config.yaml
features:
  github_environments: true

Terraform (`feature/github-repo-provisioning/`)

Environments are managed at the root module level in main.tf — not inside the vendored child module (modules/terraform-github-repository/), which must not be modified.

github_repository_environment — created/imported for all repos in all_environments_map
github_repository_environment_deployment_policy — branch_policies and tag_policies resources managed as regular resources (no import, because the provider requires numeric policy IDs which cannot be determined from YAML alone)
Import blocks for github_repository_environment from importer_tmp_dir/ only (existing environments)
Environment resources keyed as "repo:env" to avoid collisions across repos
all_environments_flattened built from generated_repos and new_repos independently — avoids merge() precedence dropping environments when a repo exists in both importer_tmp_dir/ and repos/ simultaneously

YAML configuration structure

environments:
  - environment: production
    wait_timer: 300
    can_admins_bypass: false
    prevent_self_review: true
    reviewers:
      users: ["octocat"]
      teams: ["platform-team"]   # teams must have repo access first
    deployment_policy:
      policy_type: protected_branches   # or selected_branches_and_tags

  - environment: staging
    deployment_policy:
      policy_type: selected_branches_and_tags
      branch_patterns: ["main", "release/*"]
      tag_patterns: ["v*"]

  - environment: development
    # no deployment_policy = any branch can deploy

Feature flag

features.github_environments: true in config/import-config.yaml enables environment import. Default: disabled (backward compatible).

Architecture note

The vendored module at modules/terraform-github-repository/ is a snapshot of an upstream module and must not be changed. All new Terraform resources belong in the root module (feature/github-repo-provisioning/main.tf).

Known limitations

Deployment policies are not imported on first apply — they are created fresh. Re-running apply after the first succeeds will converge state.
Maximum 6 combined users + teams as reviewers (GitHub API limitation).
Teams listed as reviewers must already have access to the repository.

References

docs/DEVELOPERS_GUIDE.md

pavlovic-ivan · 2025-11-21T10:35:48Z

docs/DEVELOPERS_GUIDE.md

+
+Configure GitHub deployment environments with protection rules and reviewers.
+
+**Import Control**: Set `feature_github_environment: true` in `import-config.yaml` to import environments.


This should be in another document, something like IMPORTER_DEVELOPERS_GUIDE.md or in the LOCAL_DEVELOPMENT_SETUP.md (if it's not already there).

The DEVELOPERS_GUIDE.md should focus only on the fields allowed to be set in a repo yaml config file

docs/DEVELOPERS_GUIDE.md

pavlovic-ivan · 2025-11-21T10:43:15Z

docs/FEATURE_GITHUB_ENVIRONMENT.md

I don't think this file is necessary. We have an example configuration file in the config template repo: https://github.com/G-Research/github-terraformer-configuration-template/blob/main/repos/repository.yaml.example

Consider moving information to the example

pavlovic-ivan · 2025-11-21T10:44:43Z

General remark: the name gcss-config-repo shouldn't be mentioned because that is arbitrary name of the configuration repo. And it's not how it's named in GR org. It can lead to confusion of new adopters

docs/GITHUB_ACTIONS_WORKFLOWS.md

docs/LOCAL_DEVELOPMENT_SETUP.md

docs/ci_flows.md

docs/workflows.md

pavlovic-ivan · 2025-11-21T10:52:12Z

feature/github-repo-importer/ADDING_FEATURES.md

Shouldn't be a part of this PR. Please move it to another PR that focuses on documentation. This one should be only about environments. Also, i don't get the purpose of this file. Maybe we go through this on a call?

pavlovic-ivan · 2025-11-21T11:00:18Z

feature/github-repo-provisioning/modules/terraform-github-repository/main.tf

Module modules/terraform-github-repository is vendored. That means it shouldn't be changed. All changes and features are added to the root module

from call note: avoid any changes to this module - move all changes outsite to pure tf files and refactor reference in it

pavlovic-ivan · 2025-11-21T11:00:31Z

feature/github-repo-provisioning/modules/terraform-github-repository/outputs.tf

Module modules/terraform-github-repository is vendored. That means it shouldn't be changed. All changes and features are added to the root module

from call note: avoid any changes to this module - move all changes outsite to pure tf files and refactor reference in it

pavlovic-ivan · 2025-11-21T11:00:40Z

feature/github-repo-provisioning/modules/terraform-github-repository/variables.tf

Module modules/terraform-github-repository is vendored. That means it shouldn't be changed. All changes and features are added to the root module

from call note: avoid any changes to this module - move all changes outsite to pure tf files and refactor reference in it

pavlovic-ivan · 2025-11-21T11:01:38Z

feature/github-repo-provisioning/main.tf

+  # ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+  # Environments Configuration
+  # ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+  environments = try(each.value.environments, [])


Once environments are moved from the child module to the root module, this should be removed.

pavlovic-ivan · 2025-11-21T11:06:43Z

feature/github-repo-provisioning/main.tf

+import {
+  for_each = local.generated_environments_map
+
+  to = module.repository[each.value.repository].github_repository_environment.environment[each.value.environment.environment]


This will change once environments are moved to root module

pavlovic-ivan · 2025-11-21T11:28:41Z

feature/github-repo-importer/pkg/github/repositories.go

+type Environment struct {
+	Environment       string                `yaml:"environment"`
+	WaitTimer         *int                  `yaml:"wait_timer,omitempty"`
+	CanAdminsBypass   *bool                 `yaml:"can_admins_bypass,omitempty"`
+	PreventSelfReview *bool                 `yaml:"prevent_self_review,omitempty"` // Extracted from ProtectionRules in API response
+	Reviewers         *EnvironmentReviewers `yaml:"reviewers,omitempty"`
+	DeploymentPolicy  *DeploymentPolicy     `yaml:"deployment_policy,omitempty"`
+}
+
+type EnvironmentReviewers struct {
+	Teams []string `yaml:"teams,omitempty"` // Team slugs (e.g., "platform-team")
+	Users []string `yaml:"users,omitempty"` // GitHub usernames (e.g., "octocat")
+}
+
+// DeploymentPolicy represents the cleaner structure for deployment policies
+type DeploymentPolicy struct {
+	PolicyType     string   `yaml:"policy_type"`                // "protected_branches" or "selected_branches_and_tags"
+	BranchPatterns []string `yaml:"branch_patterns,omitempty"`  // e.g., ["release/*", "main"] - only for selected_branches_and_tags
+	TagPatterns    []string `yaml:"tag_patterns,omitempty"`     // e.g., ["v*"] - only for selected_branches_and_tags
+}


Please move this to environments.go. And remove comments

b5b449a commit shows what changed - is this ok?

pavlovic-ivan · 2025-11-21T11:30:15Z

feature/github-repo-importer/pkg/github/github.go

+	// Log enabled features
+	if cfg != nil && cfg.Features != nil {
+		for featureName, enabled := range cfg.Features {
+			if enabled {
+				fmt.Printf("Feature enabled: %s\n", featureName)
+			}
+		}
+	}


Not needed, please remove

pavlovic-ivan · 2025-11-21T11:47:59Z

docs/DEVELOPERS_GUIDE.md

+  - **`users`**: *(string[])* GitHub usernames (max 6 total)
+  - **`teams`**: *(string[])* Team slugs (max 6 total)


It can be max 6 in total, not 6 per list

pavlovic-ivan · 2025-11-21T11:51:21Z

feature/github-repo-importer/pkg/github/constants.go

+	// Feature flags
+	// All feature flags follow the pattern: feature_<feature_name>
+	// Add new features here and they'll automatically work with the config system
+	FeatureGithubEnvironment = "feature_github_environment"


Suggested change

FeatureGithubEnvironment = "feature_github_environment"

FeatureGithubEnvironments = "feature_github_environments"

pavlovic-ivan · 2025-11-21T11:55:57Z

feature/github-repo-importer/pkg/github/config.go

+	IgnoredRepos  []string          `yaml:"ignored_repos,omitempty"`
+	SelectedRepos []string          `yaml:"selected_repos,omitempty"`
+	PageSize      *int              `yaml:"page_size,omitempty"`
+	Features      map[string]bool   `yaml:",inline"`


This line means that any field added to the root of the import-config.yaml file that has a boolean value will act as a feature flag, eg:

feature_github_environment: true some_ficticious_feature: true

Both will be captured. We want a strict configuration file that can be (de)serialized explicitly. Please change the Config struct accordingly

The best would be go to over this change in call - i made changes in f8dbedb but in short

Changes Made:

config.go - Explicit Features struct

// Before: catch-all inline map Features map[string]bool `yaml:",inline"` // After: explicit struct Features Features `yaml:"features,omitempty"` type Features struct { GithubEnvironments bool `yaml:"github_environments,omitempty"` }

config.go - Updated IsFeatureEnabled
Now uses a switch statement with explicit feature constants instead of map lookup.

constants.go - Removed the misleading comments about "add new features here and they'll automatically work".

github.go - Simplified feature check

// Before: 7 lines with nested ifs enableEnvironments := false if cfg != nil && cfg.Features != nil { if enabled, exists := cfg.Features[FeatureGithubEnvironments]; exists { enableEnvironments = enabled } } if enableEnvironments { // After: 1 line if cfg.IsFeatureEnabled(FeatureGithubEnvironments) {

New YAML format import-config.yaml:

# Before feature_github_environments: true # After features: github_environments: true

TBD @pavlovic-ivan - check proposed YAML format

@ljubon looks good!

pavlovic-ivan · 2025-11-21T11:56:42Z

feature/github-repo-importer/pkg/github/constants.go

+	// Feature flags
+	// All feature flags follow the pattern: feature_<feature_name>
+	// Add new features here and they'll automatically work with the config system


This is not correct information. Please remove it. It's also affected by the comment https://github.com/G-Research/github-terraformer/pull/28/files#r2549525571

feature/github-repo-importer/pkg/github/constants.go

pavlovic-ivan · 2025-11-21T11:59:30Z

feature/github-repo-importer/pkg/github/github.go

+	enableEnvironments := false
+	if cfg != nil && cfg.Features != nil {
+		if enabled, exists := cfg.Features[FeatureGithubEnvironment]; exists {
+			enableEnvironments = enabled
+		}
+	}
+
+	if enableEnvironments {


the three ifs can be reduced. you added a function that is not used cfg.IsFeatureEnabled that you can use to reduce the number of ifs. once that is done, you won't need enableEnvironments any more

pavlovic-ivan · 2025-11-21T12:06:30Z

feature/github-repo-importer/pkg/github/github.go

+			if err != nil {
+				if res != nil && res.StatusCode == http.StatusNotFound {
+					fmt.Printf("environments not found (this is normal for repositories without environments): %v\n", err)
+				} else {
+					fmt.Printf("failed to get environments: %v\n", err)
+				}
+				break
+			}


This should be changed so that we fail the import if getting the environments fail. Repo can not and shouldn't be partially imported, otherwise we will get drift in terraform. So it's "all or nothing" approach

pavlovic-ivan · 2025-11-21T12:06:47Z

feature/github-repo-importer/pkg/github/github.go

+						if err != nil {
+							fmt.Printf("Warning: failed to get full details for environment %s: %v\n", *env.Name, err)
+							// Use basic info if detailed fetch fails
+							allEnvironments = append(allEnvironments, env)
+						} else {
+							// Use full environment data with all details
+							allEnvironments = append(allEnvironments, fullEnv)
+						}


Similar as above, this should be changed so that we fail the import if getting the environments fail. Repo can not and shouldn't be partially imported, otherwise we will get drift in terraform. So it's "all or nothing" approach

pavlovic-ivan · 2025-11-21T12:14:10Z

feature/github-repo-importer/pkg/github/github.go

+	if err != nil {
+		fmt.Printf("Warning: failed to get organization info: %v\n", err)
+		return nil
+	}


This should be changed to a more go idiomatic approach, where you capture the error, log it, and escalate it above to the called function. Check the rest of the code to see how it's done, it's used everywhere. function resolveEnvironments will then return ([]Environment, error)

pavlovic-ivan · 2025-11-21T12:22:06Z

feature/github-repo-importer/pkg/github/github.go

+							continue
+						}
+
+						// The Reviewer field is an interface{} - try different type casts


Keep this one it's useful

pavlovic-ivan · 2025-11-21T12:23:10Z

feature/github-repo-importer/pkg/github/github.go

+									// Fallback: try map[string]interface{} for older API versions
+									if reviewerData, ok := reqReviewer.Reviewer.(map[string]interface{}); ok {
+										if login, ok := reviewerData["login"].(string); ok {
+											protectionReviewers.Users = append(protectionReviewers.Users, login)
+										}
+									}


i don't understand this. please explain

pavlovic-ivan · 2025-11-21T12:50:56Z

I will remove comments that say "remove comments", and leave only those that say "keep this comment" so that the number of asked changes here are reduced, but it also means that comments should be removed

pavlovic-ivan · 2025-11-21T12:55:45Z

feature/github-repo-importer/pkg/github/github.go

+			CanAdminsBypass: env.CanAdminsBypass,
+		}
+
+		// Extract PreventSelfReview, WaitTimer, and Reviewers from ProtectionRules


Keep this comment line, remove second and third

pavlovic-ivan · 2025-11-21T12:59:07Z

feature/github-repo-importer/pkg/github/github.go

+		// Handle reviewers at top level (fallback if not in ProtectionRules)
+		// API may return array of EnvReviewers with Type and ID
+		// We resolve IDs to human-readable names (usernames and team slugs)
+		// Note: This is usually empty as reviewers are typically in ProtectionRules
+		if env.Reviewers != nil && len(env.Reviewers) > 0 && environment.Reviewers == nil {
+			reviewers := &EnvironmentReviewers{}
+
+			// Separate reviewers by type and resolve IDs to names
+			for _, reviewer := range env.Reviewers {
+				if reviewer.Type != nil && reviewer.ID != nil {
+					switch *reviewer.Type {
+					case "Team":
+						// Resolve team ID to team slug
+						team, _, err := client.Teams.GetTeamByID(context.Background(), org.GetID(), *reviewer.ID)
+						if err != nil {
+							fmt.Printf("Warning: failed to resolve team ID %d: %v\n", *reviewer.ID, err)
+							continue
+						}
+						if team.Slug != nil {
+							reviewers.Teams = append(reviewers.Teams, *team.Slug)
+						}
+					case "User":
+						// Resolve user ID to username
+						user, _, err := client.Users.GetByID(context.Background(), *reviewer.ID)
+						if err != nil {
+							fmt.Printf("Warning: failed to resolve user ID %d: %v\n", *reviewer.ID, err)
+							continue
+						}
+						if user.Login != nil {
+							reviewers.Users = append(reviewers.Users, *user.Login)
+						}
+					}
+				}
+			}
+
+			if len(reviewers.Teams) > 0 || len(reviewers.Users) > 0 {
+				environment.Reviewers = reviewers
+			}
+		}


remove, unless you found a case where this is set. if there is one, then this block and the other on top can be merged, and reduced

pavlovic-ivan · 2025-11-21T13:01:44Z

feature/github-repo-importer/pkg/github/github.go

+			} else if env.DeploymentBranchPolicy.CustomBranchPolicies != nil && *env.DeploymentBranchPolicy.CustomBranchPolicies {
+				// Custom branch/tag patterns
+				deploymentPolicy.PolicyType = "selected_branches_and_tags"
+
+				// Fetch deployment branch policies from GitHub API
+				branchPatterns, tagPatterns := fetchDeploymentPolicies(client, owner, repo, env.GetName())
+
+				if len(branchPatterns) > 0 {
+					deploymentPolicy.BranchPatterns = branchPatterns
+				}
+				if len(tagPatterns) > 0 {
+					deploymentPolicy.TagPatterns = tagPatterns
+				}
+			}


This needs an else statement that should raise an error, because it means someone tampered with the json file and introduced something we don't know exists, or Github made a change, and we should be aware if any of the two happened. If so, throw an error, and fail the import.

pavlovic-ivan · 2025-11-21T13:02:50Z

feature/github-repo-importer/pkg/github/github.go

+				if len(branchPatterns) > 0 {
+					deploymentPolicy.BranchPatterns = branchPatterns
+				}
+				if len(tagPatterns) > 0 {
+					deploymentPolicy.TagPatterns = tagPatterns
+				}


These two should work together and check if length of both arrays in total is bigger than 6. If yes, throw an error, and fail the import.

pavlovic-ivan

Please take a look at comments and change accordingly

Co-authored-by: Ivan Pavlovic <ivan@gr-oss.io>