Skip to content

Commit 16c1e02

Browse files
deigastevehipwell
deiga
and
stevehipwell
authored
[MAINT][FEAT] Fix Org Ruleset tests && enable allowed_merge_methods (#2976)
* Switch from `NewSubsystemLoggingHTTPTransport` to `NewLoggingHTTPTransport` Updated the logging transport in the RateLimitedHTTPClient function to use NewLoggingHTTPTransport. As the subsystem would need to be explicitly inititated in each resource Signed-off-by: Timo Sand <[email protected]> * Ensure `update_allows_fetch_and_merge` isn't added for Org Ruleset Signed-off-by: Timo Sand <[email protected]> * Need to have one of `repository_name` or `repository_id` defined Signed-off-by: Timo Sand <[email protected]> * Update indentation Signed-off-by: Timo Sand <[email protected]> * Add handling of `AllowedMergeMethods` It's a required field in the API and go-github doesn't use `omitempty`, so it submits `nil` if it isn't sent explicitly. This change tries to keep it in the state, without having a configuration option for it (poor choice?) And it defaults to all 3 available merge methods if it can't set something from the state. Signed-off-by: Timo Sand <[email protected]> * Add `allowed_merge_methods` to `rules` for Org & Repo rulesets This is a required field and the SDK doesn't omit if it's empty Signed-off-by: Timo Sand <[email protected]> * Fixed type conversion for `allowed_merge_methods` Signed-off-by: Timo Sand <[email protected]> * Update test content to actually pass the GH API Signed-off-by: Timo Sand <[email protected]> * Enable `TestGithubOrganizationRulesets/Creates_and_updates_organization_rulesets_without_errors` to pass Signed-off-by: Timo Sand <[email protected]> * Add workaround for GH API bug that OrgAdmin actor_id is returned as `null` Signed-off-by: Timo Sand <[email protected]> * `make fmt` Signed-off-by: Timo Sand <[email protected]> * Fix `TestGithubOrganizationRulesets/Creates_organization_ruleset_with_all_bypass_modes` Main fix: `bypass_actors` is returned as sorted from GH API so tests need re-indexing Signed-off-by: Timo Sand <[email protected]> * Update github/resource_github_organization_ruleset_test.go Co-authored-by: Steve Hipwell <[email protected]> * Update github/resource_github_organization_ruleset_test.go Co-authored-by: Steve Hipwell <[email protected]> * Update github/resource_github_organization_ruleset_test.go Co-authored-by: Steve Hipwell <[email protected]> * Fix casing of constant Signed-off-by: Timo Sand <[email protected]> * Fix attribute reference Signed-off-by: Timo Sand <[email protected]> * Refactor `Providers` => `ProviderFactories` in org ruleset tests Signed-off-by: Timo Sand <[email protected]> * Remove "normalizing" `actor_id`` With the recent changes to the SDK and the Create method we less often purely refresh state if it hasn't changed upstream, leading to us ignoring the return value of `actor_id` often Signed-off-by: Timo Sand <[email protected]> * Refactor to use `tflog` in `resource_github_organization_ruleset.go` Signed-off-by: Timo Sand <[email protected]> --------- Signed-off-by: Timo Sand <[email protected]> Co-authored-by: Steve Hipwell <[email protected]>
1 parent 569610a commit 16c1e02

5 files changed

Lines changed: 247 additions & 41 deletions

‎github/config.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ var (
5959
func RateLimitedHTTPClient(client *http.Client, writeDelay, readDelay, retryDelay time.Duration, parallelRequests bool, retryableErrors map[int]bool, maxRetries int) *http.Client {
6060
client.Transport = NewEtagTransport(client.Transport)
6161
client.Transport = NewRateLimitTransport(client.Transport, WithWriteDelay(writeDelay), WithReadDelay(readDelay), WithParallelRequests(parallelRequests))
62-
client.Transport = logging.NewSubsystemLoggingHTTPTransport("GitHub", client.Transport)
62+
client.Transport = logging.NewLoggingHTTPTransport(client.Transport)
6363
client.Transport = newPreviewHeaderInjectorTransport(map[string]string{
6464
// TODO: remove when Stone Crop preview is moved to general availability in the GraphQL API
6565
"Accept": "application/vnd.github.stone-crop-preview+json",

‎github/resource_github_organization_ruleset.go‎

Lines changed: 130 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,11 @@ import (
44
"context"
55
"errors"
66
"fmt"
7-
"log"
87
"net/http"
98
"strconv"
109

1110
"github.com/google/go-github/v81/github"
11+
"github.com/hashicorp/terraform-plugin-log/tflog"
1212
"github.com/hashicorp/terraform-plugin-sdk/v2/diag"
1313
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
1414
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation"
@@ -46,7 +46,7 @@ func resourceGithubOrganizationRuleset() *schema.Resource {
4646
Description: "Possible values for Enforcement are `disabled`, `active`, `evaluate`. Note: `evaluate` is currently only supported for owners of type `organization`.",
4747
},
4848
"bypass_actors": {
49-
Type: schema.TypeList,
49+
Type: schema.TypeList, // TODO: These are returned from GH API sorted by actor_id, we might want to investigate if we want to include sorting
5050
Optional: true,
5151
DiffSuppressFunc: bypassActorsDiffSuppressFunc,
5252
Description: "The actors that can bypass the rules in this ruleset.",
@@ -198,6 +198,16 @@ func resourceGithubOrganizationRuleset() *schema.Resource {
198198
Description: "Require all commits be made to a non-target branch and submitted via a pull request before they can be merged.",
199199
Elem: &schema.Resource{
200200
Schema: map[string]*schema.Schema{
201+
"allowed_merge_methods": {
202+
Type: schema.TypeList,
203+
Required: true,
204+
MinItems: 1,
205+
Description: "Array of allowed merge methods. Allowed values include `merge`, `squash`, and `rebase`. At least one option must be enabled.",
206+
Elem: &schema.Schema{
207+
Type: schema.TypeString,
208+
ValidateDiagFunc: toDiagFunc(validation.StringInSlice([]string{"merge", "squash", "rebase"}, false), "allowed_merge_methods"),
209+
},
210+
},
201211
"dismiss_stale_reviews_on_push": {
202212
Type: schema.TypeBool,
203213
Optional: true,
@@ -588,13 +598,23 @@ func resourceGithubOrganizationRuleset() *schema.Resource {
588598

589599
func resourceGithubOrganizationRulesetCreate(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics {
590600
client := meta.(*Owner).v3client
591-
592601
owner := meta.(*Owner).name
602+
name := d.Get("name").(string)
603+
604+
tflog.Debug(ctx, fmt.Sprintf("Creating organization ruleset: %s/%s", owner, name), map[string]any{
605+
"owner": owner,
606+
"name": name,
607+
})
593608

594609
rulesetReq := resourceGithubRulesetObject(d, owner)
595610

596611
ruleset, resp, err := client.Organizations.CreateRepositoryRuleset(ctx, owner, rulesetReq)
597612
if err != nil {
613+
tflog.Error(ctx, fmt.Sprintf("Failed to create organization ruleset: %s/%s", owner, name), map[string]any{
614+
"owner": owner,
615+
"name": name,
616+
"error": err.Error(),
617+
})
598618
return diag.FromErr(err)
599619
}
600620

@@ -603,16 +623,31 @@ func resourceGithubOrganizationRulesetCreate(ctx context.Context, d *schema.Reso
603623
_ = d.Set("node_id", ruleset.GetNodeID())
604624
_ = d.Set("etag", resp.Header.Get("ETag"))
605625

626+
tflog.Info(ctx, fmt.Sprintf("Created organization ruleset: %s/%s (ID: %d)", owner, name, *ruleset.ID), map[string]any{
627+
"owner": owner,
628+
"name": name,
629+
"ruleset_id": *ruleset.ID,
630+
})
631+
606632
return nil
607633
}
608634

609635
func resourceGithubOrganizationRulesetRead(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics {
610636
client := meta.(*Owner).v3client
611-
612637
owner := meta.(*Owner).name
613638

639+
tflog.Trace(ctx, fmt.Sprintf("Reading organization ruleset: %s", d.Id()), map[string]any{
640+
"owner": owner,
641+
"ruleset_id": d.Id(),
642+
})
643+
614644
rulesetID, err := strconv.ParseInt(d.Id(), 10, 64)
615645
if err != nil {
646+
tflog.Error(ctx, fmt.Sprintf("Could not convert ruleset ID '%s' to int64", d.Id()), map[string]any{
647+
"owner": owner,
648+
"ruleset_id": d.Id(),
649+
"error": err.Error(),
650+
})
616651
return diag.FromErr(unconvertibleIdErr(d.Id(), err))
617652
}
618653

@@ -625,15 +660,26 @@ func resourceGithubOrganizationRulesetRead(ctx context.Context, d *schema.Resour
625660
var ghErr *github.ErrorResponse
626661
if errors.As(err, &ghErr) {
627662
if ghErr.Response.StatusCode == http.StatusNotModified {
663+
tflog.Debug(ctx, "API responded with StatusNotModified, not refreshing state", map[string]any{
664+
"owner": owner,
665+
"ruleset_id": rulesetID,
666+
})
628667
return nil
629668
}
630669
if ghErr.Response.StatusCode == http.StatusNotFound {
631-
log.Printf("[INFO] Removing ruleset %s: %d from state because it no longer exists in GitHub",
632-
owner, rulesetID)
670+
tflog.Info(ctx, fmt.Sprintf("Removing ruleset %s/%d from state because it no longer exists in GitHub", owner, rulesetID), map[string]any{
671+
"owner": owner,
672+
"ruleset_id": rulesetID,
673+
})
633674
d.SetId("")
634675
return nil
635676
}
636677
}
678+
tflog.Error(ctx, fmt.Sprintf("Failed to read organization ruleset: %s/%d", owner, rulesetID), map[string]any{
679+
"owner": owner,
680+
"ruleset_id": rulesetID,
681+
"error": err.Error(),
682+
})
637683
return diag.FromErr(err)
638684
}
639685

@@ -647,23 +693,45 @@ func resourceGithubOrganizationRulesetRead(ctx context.Context, d *schema.Resour
647693
_ = d.Set("node_id", ruleset.GetNodeID())
648694
_ = d.Set("etag", resp.Header.Get("ETag"))
649695

696+
tflog.Trace(ctx, fmt.Sprintf("Successfully read organization ruleset: %s/%d", owner, rulesetID), map[string]any{
697+
"owner": owner,
698+
"ruleset_id": rulesetID,
699+
"name": ruleset.Name,
700+
})
701+
650702
return nil
651703
}
652704

653705
func resourceGithubOrganizationRulesetUpdate(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics {
654706
client := meta.(*Owner).v3client
655-
656707
owner := meta.(*Owner).name
657-
658-
rulesetReq := resourceGithubRulesetObject(d, owner)
708+
name := d.Get("name").(string)
659709

660710
rulesetID, err := strconv.ParseInt(d.Id(), 10, 64)
661711
if err != nil {
712+
tflog.Error(ctx, fmt.Sprintf("Could not convert ruleset ID '%s' to int64", d.Id()), map[string]any{
713+
"owner": owner,
714+
"ruleset_id": d.Id(),
715+
"error": err.Error(),
716+
})
662717
return diag.FromErr(unconvertibleIdErr(d.Id(), err))
663718
}
664719

720+
tflog.Debug(ctx, fmt.Sprintf("Updating organization ruleset: %s/%d", owner, rulesetID), map[string]any{
721+
"owner": owner,
722+
"ruleset_id": rulesetID,
723+
"name": name,
724+
})
725+
726+
rulesetReq := resourceGithubRulesetObject(d, owner)
727+
665728
ruleset, resp, err := client.Organizations.UpdateRepositoryRuleset(ctx, owner, rulesetID, rulesetReq)
666729
if err != nil {
730+
tflog.Error(ctx, fmt.Sprintf("Failed to update organization ruleset: %s/%d", owner, rulesetID), map[string]any{
731+
"owner": owner,
732+
"ruleset_id": rulesetID,
733+
"error": err.Error(),
734+
})
667735
return diag.FromErr(err)
668736
}
669737

@@ -672,6 +740,12 @@ func resourceGithubOrganizationRulesetUpdate(ctx context.Context, d *schema.Reso
672740
_ = d.Set("node_id", ruleset.GetNodeID())
673741
_ = d.Set("etag", resp.Header.Get("ETag"))
674742

743+
tflog.Info(ctx, fmt.Sprintf("Updated organization ruleset: %s/%d", owner, rulesetID), map[string]any{
744+
"owner": owner,
745+
"ruleset_id": rulesetID,
746+
"name": name,
747+
})
748+
675749
return nil
676750
}
677751

@@ -681,36 +755,79 @@ func resourceGithubOrganizationRulesetDelete(ctx context.Context, d *schema.Reso
681755

682756
rulesetID, err := strconv.ParseInt(d.Id(), 10, 64)
683757
if err != nil {
758+
tflog.Error(ctx, fmt.Sprintf("Could not convert ruleset ID '%s' to int64", d.Id()), map[string]any{
759+
"owner": owner,
760+
"ruleset_id": d.Id(),
761+
"error": err.Error(),
762+
})
684763
return diag.FromErr(unconvertibleIdErr(d.Id(), err))
685764
}
686765

687-
log.Printf("[DEBUG] Deleting organization ruleset: %s: %d", owner, rulesetID)
766+
tflog.Debug(ctx, fmt.Sprintf("Deleting organization ruleset: %s/%d", owner, rulesetID), map[string]any{
767+
"owner": owner,
768+
"ruleset_id": rulesetID,
769+
})
770+
688771
_, err = client.Organizations.DeleteRepositoryRuleset(ctx, owner, rulesetID)
689772
if err != nil {
773+
tflog.Error(ctx, fmt.Sprintf("Failed to delete organization ruleset: %s/%d", owner, rulesetID), map[string]any{
774+
"owner": owner,
775+
"ruleset_id": rulesetID,
776+
"error": err.Error(),
777+
})
690778
return diag.FromErr(err)
691779
}
692780

781+
tflog.Info(ctx, fmt.Sprintf("Deleted organization ruleset: %s/%d", owner, rulesetID), map[string]any{
782+
"owner": owner,
783+
"ruleset_id": rulesetID,
784+
})
785+
693786
return nil
694787
}
695788

696789
func resourceGithubOrganizationRulesetImport(ctx context.Context, d *schema.ResourceData, meta any) ([]*schema.ResourceData, error) {
790+
client := meta.(*Owner).v3client
791+
owner := meta.(*Owner).name
792+
697793
rulesetID, err := strconv.ParseInt(d.Id(), 10, 64)
698794
if err != nil {
795+
tflog.Error(ctx, fmt.Sprintf("Could not convert ruleset ID '%s' to int64", d.Id()), map[string]any{
796+
"owner": owner,
797+
"ruleset_id": d.Id(),
798+
"error": err.Error(),
799+
})
699800
return []*schema.ResourceData{d}, unconvertibleIdErr(d.Id(), err)
700801
}
701802
if rulesetID == 0 {
803+
tflog.Error(ctx, "ruleset_id must be present and non-zero", map[string]any{
804+
"owner": owner,
805+
"ruleset_id": rulesetID,
806+
})
702807
return []*schema.ResourceData{d}, fmt.Errorf("`ruleset_id` must be present")
703808
}
704-
log.Printf("[DEBUG] Importing organization ruleset with ID: %d", rulesetID)
705809

706-
client := meta.(*Owner).v3client
707-
owner := meta.(*Owner).name
810+
tflog.Debug(ctx, fmt.Sprintf("Importing organization ruleset: %s/%d", owner, rulesetID), map[string]any{
811+
"owner": owner,
812+
"ruleset_id": rulesetID,
813+
})
708814

709815
ruleset, _, err := client.Organizations.GetRepositoryRuleset(ctx, owner, rulesetID)
710816
if ruleset == nil || err != nil {
817+
tflog.Error(ctx, fmt.Sprintf("Failed to import organization ruleset: %s/%d", owner, rulesetID), map[string]any{
818+
"owner": owner,
819+
"ruleset_id": rulesetID,
820+
"error": err.Error(),
821+
})
711822
return []*schema.ResourceData{d}, err
712823
}
713824
d.SetId(strconv.FormatInt(ruleset.GetID(), 10))
714825

826+
tflog.Info(ctx, fmt.Sprintf("Imported organization ruleset: %s/%d (name: %s)", owner, rulesetID, ruleset.Name), map[string]any{
827+
"owner": owner,
828+
"ruleset_id": rulesetID,
829+
"name": ruleset.Name,
830+
})
831+
715832
return []*schema.ResourceData{d}, nil
716833
}

0 commit comments

Comments
 (0)