integrations / terraform-provider-github

Terraform GitHub provider
https://www.terraform.io/docs/providers/github/
MIT License
870 stars 709 forks source link

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

Open stevehipwell opened 8 months ago

stevehipwell commented 8 months ago

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

github-actions[bot] commented 8 months ago

👋 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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

@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.