[DOCS]: What is the difference between `github_repository_deployment_branch_policy` & `github_repository_environment_deployment_policy`

[stevehipwell]

Describe the need

The github_repository_deployment_branch_policy & github_repository_environment_deployment_policy resources look like they do exactly the same thing; so firstly is this correct? If so could the documentation reflect this and provide guidance on which one to use?

SDK Version

No response

API Version

No response

Relevant log output

No response

Code of Conduct

• I agree to follow this project's Code of Conduct

[github-actions]

👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labled with Status: Up for grabs. You & others like you are the reason all of this works! So thank you & happy coding! 🚀

[filip-zyzniewski]

The code is quite similar after moving one function and seding:

Filip.Zyzniewski@JFYYQW9DJ7 terraform-provider-github % git rev-parse HEAD
99d19370796086f41a293563ceceb7b37ce521c5
Filip.Zyzniewski@JFYYQW9DJ7 terraform-provider-github % git diff --color-moved=blocks github/resource_github_repository_deployment_branch_policy.go
diff --git a/github/resource_github_repository_deployment_branch_policy.go b/github/resource_github_repository_deployment_branch_policy.go
index c2c1e558..b119da1a 100644
--- a/github/resource_github_repository_deployment_branch_policy.go
+++ b/github/resource_github_repository_deployment_branch_policy.go
@@ -47,27 +47,6 @@ func resourceGithubRepositoryDeploymentBranchPolicy() *schema.Resource {
        }
 }
 
-func resourceGithubRepositoryDeploymentBranchPolicyUpdate(d *schema.ResourceData, meta interface{}) error {
-       ctx := context.Background()
-       client := meta.(*Owner).v3client
-       owner := meta.(*Owner).name
-       repoName := d.Get("repository").(string)
-       environmentName := d.Get("environment_name").(string)
-       name := d.Get("name").(string)
-
-       id, err := strconv.Atoi(d.Id())
-       if err != nil {
-               return err
-       }
-
-       _, _, err = client.Repositories.UpdateDeploymentBranchPolicy(ctx, owner, repoName, environmentName, int64(id), &github.DeploymentBranchPolicyRequest{Name: &name})
-       if err != nil {
-               return err
-       }
-
-       return resourceGithubRepositoryDeploymentBranchPolicyRead(d, meta)
-}
-
 func resourceGithubRepositoryDeploymentBranchPolicyCreate(d *schema.ResourceData, meta interface{}) error {
        ctx := context.Background()
        if !d.IsNewResource() {
@@ -138,6 +117,27 @@ func resourceGithubRepositoryDeploymentBranchPolicyRead(d *schema.ResourceData,
        return nil
 }
 
+func resourceGithubRepositoryDeploymentBranchPolicyUpdate(d *schema.ResourceData, meta interface{}) error {
+       ctx := context.Background()
+       client := meta.(*Owner).v3client
+       owner := meta.(*Owner).name
+       repoName := d.Get("repository").(string)
+       environmentName := d.Get("environment_name").(string)
+       name := d.Get("name").(string)
+
+       id, err := strconv.Atoi(d.Id())
+       if err != nil {
+               return err
+       }
+
+       _, _, err = client.Repositories.UpdateDeploymentBranchPolicy(ctx, owner, repoName, environmentName, int64(id), &github.DeploymentBranchPolicyRequest{Name: &name})
+       if err != nil {
+               return err
+       }
+
+       return resourceGithubRepositoryDeploymentBranchPolicyRead(d, meta)
+}
+
 func resourceGithubRepositoryDeploymentBranchPolicyDelete(d *schema.ResourceData, meta interface{}) error {
        ctx := context.WithValue(context.Background(), ctxId, d.Id())
 
Filip.Zyzniewski@JFYYQW9DJ7 terraform-provider-github % diff -u <(sed 's:GithubRepositoryDeploymentBranchPolicy:GithubRepositoryEnvironmentDeploymentPolicy:g' ./github/resource_github_repository_deployment_branch_policy.go) ./github/resource_github_repository_environment_deployment_policy.go
--- /dev/fd/11  2024-07-03 17:24:46
+++ ./github/resource_github_repository_environment_deployment_policy.go        2024-07-03 17:12:47
@@ -4,6 +4,7 @@
        "context"
        "log"
        "net/http"
+       "net/url"
        "strconv"
 
        "github.com/google/go-github/v57/github"
@@ -17,83 +18,79 @@
                Update: resourceGithubRepositoryEnvironmentDeploymentPolicyUpdate,
                Delete: resourceGithubRepositoryEnvironmentDeploymentPolicyDelete,
                Importer: &schema.ResourceImporter{
-                       State: resourceGithubRepositoryEnvironmentDeploymentPolicyImport,
+                       StateContext: schema.ImportStatePassthroughContext,
                },
-
                Schema: map[string]*schema.Schema{
                        "repository": {
                                Type:        schema.TypeString,
                                Required:    true,
                                ForceNew:    true,
-                               Description: "The GitHub repository name.",
+                               Description: "The name of the repository. The name is not case sensitive.",
                        },
-                       "environment_name": {
+                       "environment": {
                                Type:        schema.TypeString,
                                Required:    true,
                                ForceNew:    true,
-                               Description: "The target environment name.",
+                               Description: "The name of the environment.",
                        },
-                       "name": {
+                       "branch_pattern": {
                                Type:        schema.TypeString,
                                Required:    true,
-                               Description: "The name of the branch",
+                               ForceNew:    false,
+                               Description: "The name pattern that branches must match in order to deploy to the environment.",
                        },
-                       "etag": {
-                               Type:        schema.TypeString,
-                               Computed:    true,
-                               Description: "An etag representing the Branch object.",
-                       },
                },
        }
+
 }
 
 func resourceGithubRepositoryEnvironmentDeploymentPolicyCreate(d *schema.ResourceData, meta interface{}) error {
+       client := meta.(*Owner).v3client
        ctx := context.Background()
-       if !d.IsNewResource() {
-               ctx = context.WithValue(ctx, ctxId, d.Id())
-       }
 
-       client := meta.(*Owner).v3client
        owner := meta.(*Owner).name
        repoName := d.Get("repository").(string)
-       environmentName := d.Get("environment_name").(string)
-       name := d.Get("name").(string)
+       envName := d.Get("environment").(string)
+       branchPattern := d.Get("branch_pattern").(string)
+       escapedEnvName := url.PathEscape(envName)
 
-       policy, _, err := client.Repositories.CreateDeploymentBranchPolicy(ctx, owner, repoName, environmentName, &github.DeploymentBranchPolicyRequest{Name: &name})
+       createData := github.DeploymentBranchPolicyRequest{
+               Name: github.String(branchPattern),
+       }
+
+       resultKey, _, err := client.Repositories.CreateDeploymentBranchPolicy(ctx, owner, repoName, escapedEnvName, &createData)
        if err != nil {
                return err
        }
 
-       d.SetId(strconv.FormatInt(*policy.ID, 10))
-
+       d.SetId(buildThreePartID(repoName, escapedEnvName, strconv.FormatInt(resultKey.GetID(), 10)))
        return resourceGithubRepositoryEnvironmentDeploymentPolicyRead(d, meta)
 }
 
 func resourceGithubRepositoryEnvironmentDeploymentPolicyRead(d *schema.ResourceData, meta interface{}) error {
+       client := meta.(*Owner).v3client
        ctx := context.WithValue(context.Background(), ctxId, d.Id())
-       if !d.IsNewResource() {
-               ctx = context.WithValue(ctx, ctxEtag, d.Get("etag").(string))
-       }
 
-       client := meta.(*Owner).v3client
        owner := meta.(*Owner).name
-       repoName := d.Get("repository").(string)
-       environmentName := d.Get("environment_name").(string)
+       repoName, envName, branchPolicyIdString, err := parseThreePartID(d.Id(), "repository", "environment", "branchPolicyId")
+       if err != nil {
+               return err
+       }
 
-       id, err := strconv.Atoi(d.Id())
+       branchPolicyId, err := strconv.ParseInt(branchPolicyIdString, 10, 64)
        if err != nil {
                return err
        }
 
-       policy, resp, err := client.Repositories.GetDeploymentBranchPolicy(ctx, owner, repoName, environmentName, int64(id))
+       branchPolicy, _, err := client.Repositories.GetDeploymentBranchPolicy(ctx, owner, repoName, envName, branchPolicyId)
        if err != nil {
                if ghErr, ok := err.(*github.ErrorResponse); ok {
                        if ghErr.Response.StatusCode == http.StatusNotModified {
                                return nil
                        }
                        if ghErr.Response.StatusCode == http.StatusNotFound {
-                               log.Printf("[INFO] Removing deployment branch policy for environment %s: %s from state because it no longer exists in GitHub",
-                                       repoName, environmentName)
+                               log.Printf("[INFO] Removing branch deployment policy for %s/%s/%s from state because it no longer exists in GitHub",
+                                       owner, repoName, envName)
                                d.SetId("")
                                return nil
                        }
@@ -101,81 +98,60 @@
                return err
        }
 
-       if err = d.Set("etag", resp.Header.Get("ETag")); err != nil {
-               return err
-       }
-       if err = d.Set("repository", repoName); err != nil {
-               return err
-       }
-       if err = d.Set("environment_name", environmentName); err != nil {
-               return err
-       }
-       if err = d.Set("name", policy.Name); err != nil {
-               return err
-       }
-
+       d.Set("branch_pattern", branchPolicy.GetName())
        return nil
 }
 
 func resourceGithubRepositoryEnvironmentDeploymentPolicyUpdate(d *schema.ResourceData, meta interface{}) error {
-       ctx := context.Background()
        client := meta.(*Owner).v3client
+       ctx := context.Background()
+
        owner := meta.(*Owner).name
        repoName := d.Get("repository").(string)
-       environmentName := d.Get("environment_name").(string)
-       name := d.Get("name").(string)
-
-       id, err := strconv.Atoi(d.Id())
+       envName := d.Get("environment").(string)
+       branchPattern := d.Get("branch_pattern").(string)
+       escapedEnvName := url.PathEscape(envName)
+       _, _, branchPolicyIdString, err := parseThreePartID(d.Id(), "repository", "environment", "branchPolicyId")
        if err != nil {
                return err
        }
 
-       _, _, err = client.Repositories.UpdateDeploymentBranchPolicy(ctx, owner, repoName, environmentName, int64(id), &github.DeploymentBranchPolicyRequest{Name: &name})
+       branchPolicyId, err := strconv.ParseInt(branchPolicyIdString, 10, 64)
        if err != nil {
                return err
        }
 
+       updateData := github.DeploymentBranchPolicyRequest{
+               Name: github.String(branchPattern),
+       }
+
+       resultKey, _, err := client.Repositories.UpdateDeploymentBranchPolicy(ctx, owner, repoName, escapedEnvName, branchPolicyId, &updateData)
+       if err != nil {
+               return err
+       }
+       d.SetId(buildThreePartID(repoName, escapedEnvName, strconv.FormatInt(resultKey.GetID(), 10)))
        return resourceGithubRepositoryEnvironmentDeploymentPolicyRead(d, meta)
 }
 
 func resourceGithubRepositoryEnvironmentDeploymentPolicyDelete(d *schema.ResourceData, meta interface{}) error {
-       ctx := context.WithValue(context.Background(), ctxId, d.Id())
-
        client := meta.(*Owner).v3client
-       owner := meta.(*Owner).name
-       repoName := d.Get("repository").(string)
-       environmentName := d.Get("environment_name").(string)
+       ctx := context.Background()
 
-       id, err := strconv.Atoi(d.Id())
+       owner := meta.(*Owner).name
+       repoName, envName, branchPolicyIdString, err := parseThreePartID(d.Id(), "repository", "environment", "branchPolicyId")
        if err != nil {
                return err
        }
 
-       _, error := client.Repositories.DeleteDeploymentBranchPolicy(ctx, owner, repoName, environmentName, int64(id))
-       if error != nil {
-               return error
-       }
-       return nil
-}
-
-func resourceGithubRepositoryEnvironmentDeploymentPolicyImport(d *schema.ResourceData, meta interface{}) ([]*schema.ResourceData, error) {
-       repoName, environmentName, id, err := parseThreePartID(d.Id(), "repository", "environment_name", "id")
+       branchPolicyId, err := strconv.ParseInt(branchPolicyIdString, 10, 64)
        if err != nil {
-               return nil, err
+               return err
        }
 
-       d.SetId(id)
-       if err = d.Set("repository", repoName); err != nil {
-               return nil, err
-       }
-       if err = d.Set("environment_name", environmentName); err != nil {
-               return nil, err
-       }
-
-       err = resourceGithubRepositoryEnvironmentDeploymentPolicyRead(d, meta)
+       _, err = client.Repositories.DeleteDeploymentBranchPolicy(ctx, owner, repoName, envName, branchPolicyId)
        if err != nil {
-               return nil, err
+               return err
        }
 
-       return []*schema.ResourceData{d}, nil
+       return nil
 }
Filip.Zyzniewski@JFYYQW9DJ7 terraform-provider-github %

[kfcampbell]

This is an oversight on our part. I've added the nNext label here; for the next breaking change we may consolidate resources and migrate to a single one. If either of you would like to submit a PR, please feel free!

[stevehipwell]

@kfcampbell which one would be a candidate for removal? That one should at least be documented as deprecated now to remove confusion and so when it is removed it has the smallest possible impact. When I looked at both I decided to use the github_repository_deployment_branch_policy resource as I thought it aligned closest to the rest of the API.

[badbort]

@kfcampbell any update on this? Similar scenario where there is confusion between which combination I should be using. There's the two terraform resources as mentioned earlier, but there's also the deployment_branch_policy block in github_repository_environment

[stevehipwell]

@badbort you need to set custom_branch_policies to true in the deployment_branch_policy block in the github_repository_environment resource to be able to configure a deployment policy.

@kfcampbell I think the github_repository_environment_deployment_policy is the resource which should stay as it has support for the whole API (tags and branches). I'll open a PR to deprecate the github_repository_deployment_branch_policy resource.

[sumnerwarren]

I'm happy to help move this forward, but I'm personally stalled by indecision. Here's what I'm thinking about:

• GitHub calls these objects "Deployment Branch Policies". Given the support for tags, that feels like a misnomer now. I think the more generic environment_deployment_policies makes sense personally, but there is probably some value for future contributors in matching the name used in the GitHub API docs.
• If we decide to keep github_repository_environment_deployment_policy instead, should we also replace data_source_github_repository_deployment_branch_policies with a new data_source_github_repository_environment_deployment_policies so the names are consistent?
• In looking at [BUG]: Importing github_repository_environment_deployment_policy resource does not populate repository and environment fields. #2012, I noticed there is a bug related to escaping in github_repository_environment_deployment_policy. We are storing the escaped version of the environment name in the resource's id attribute. This does not match how we're handling other environment-scoped resources and causes issues in the Read/Update/Delete operations for impacted resources (Create works fine).
• We can ignore the escaping issue in the CRUD operations (see here), but I can't figure out how to write a migration for github_repository_environment_deployment_policy that only impacts a resource's id attribute. The migration seems to work during planning, but the results do not get persisted in the state. So unless a user updates an existing deployment policy by changing the pattern, the stored id will continue to have the escaped environment name. I don't think that actually presents any issues, but I'm not 100% sure.
• We could write a migration for github_repository_deployment_branch_policy from name: "foo" to branch_pattern: "foo" while adding tag support.

All of that makes me lean toward adding tag support to github_respository_deployment_branch_policy and deprecating github_repository_environment_deployment_policy. I think a maintainer (@kfcampbell?) probably needs to weigh in on which way to proceed here and then I'm happy to work on PRs as necessary.