Skip to content
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

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

Open
1 task done
stevehipwell opened this issue Nov 3, 2023 · 4 comments
Labels
Status: Up for grabs Issues that are ready to be worked on by anyone Type: Documentation Improvements or additions to documentation vNext

Comments

@stevehipwell
Copy link
Contributor

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
@stevehipwell stevehipwell added Status: Triage This is being looked at and prioritized Type: Documentation Improvements or additions to documentation labels Nov 3, 2023
Copy link

github-actions bot commented Nov 3, 2023

👋 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! 🚀

@kfcampbell kfcampbell added Status: Up for grabs Issues that are ready to be worked on by anyone and removed Status: Triage This is being looked at and prioritized labels Nov 7, 2023
@filip-zyzniewski
Copy link

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
Copy link
Member

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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Up for grabs Issues that are ready to be worked on by anyone Type: Documentation Improvements or additions to documentation vNext
Projects
None yet
Development

No branches or pull requests

3 participants