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

🏳️ Custom properties resource & data #2107

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 71 additions & 0 deletions github/data_source_github_organization_custom_properties.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
package github

import (
"context"
"fmt"

"github.com/hashicorp/terraform-plugin-sdk/helper/schema"
)

func dataSourceGithubOrganizationCustomProperties() *schema.Resource {
return &schema.Resource{
Read: dataSourceGithubOrganizationCustomPropertiesRead,

Schema: map[string]*schema.Schema{
"property_name": {
Type: schema.TypeString,
Required: true,
},
"value_type": {
Type: schema.TypeString,
Optional: true,
},
"required": {
Type: schema.TypeBool,
Optional: true,
},
"default_value": {
Type: schema.TypeString,
Optional: true,
Computed: true,
},
"description": {
Type: schema.TypeString,
Optional: true,
Computed: true,
},
"allowed_values": {
Type: schema.TypeList,
Optional: true,
Computed: true,
Elem: &schema.Schema{Type: schema.TypeString},
},
},
}
}

func dataSourceGithubOrganizationCustomPropertiesRead(d *schema.ResourceData, meta interface{}) error {
client := meta.(*Owner).v3client
ctx := context.Background()
orgName := meta.(*Owner).name

err := checkOrganization(meta)
if err != nil {
return err
}

propertyAttributes, _, err := client.Organizations.GetCustomProperty(ctx, orgName, d.Get("property_name").(string))
if err != nil {
return fmt.Errorf("error querying GitHub custom properties %s: %s", orgName, err)
}

d.SetId("org-custom-properties")
d.Set("allowed_values", propertyAttributes.AllowedValues)
d.Set("default_value", propertyAttributes.DefaultValue)
d.Set("description", propertyAttributes.Description)
d.Set("property_name", propertyAttributes.PropertyName)
d.Set("required", propertyAttributes.Required)
d.Set("value_type", propertyAttributes.ValueType)

return nil
}
2 changes: 2 additions & 0 deletions github/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ func Provider() terraform.ResourceProvider {
"github_membership": resourceGithubMembership(),
"github_organization_block": resourceOrganizationBlock(),
"github_organization_custom_role": resourceGithubOrganizationCustomRole(),
"github_organization_custom_properties": resourceGithubOrganizationCustomProperties(),
"github_organization_project": resourceGithubOrganizationProject(),
"github_organization_security_manager": resourceGithubOrganizationSecurityManager(),
"github_organization_ruleset": resourceGithubOrganizationRuleset(),
Expand Down Expand Up @@ -202,6 +203,7 @@ func Provider() terraform.ResourceProvider {
"github_membership": dataSourceGithubMembership(),
"github_organization": dataSourceGithubOrganization(),
"github_organization_custom_role": dataSourceGithubOrganizationCustomRole(),
"github_organization_custom_properties": dataSourceGithubOrganizationCustomProperties(),
"github_organization_external_identities": dataSourceGithubOrganizationExternalIdentities(),
"github_organization_ip_allow_list": dataSourceGithubOrganizationIpAllowList(),
"github_organization_team_sync_groups": dataSourceGithubOrganizationTeamSyncGroups(),
Expand Down
141 changes: 141 additions & 0 deletions github/resource_github_organisation_custom_properties.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,141 @@
package github

import (
"context"

"github.com/google/go-github/v57/github"
"github.com/hashicorp/terraform-plugin-sdk/helper/customdiff"
"github.com/hashicorp/terraform-plugin-sdk/helper/schema"
)

func resourceGithubOrganizationCustomProperties() *schema.Resource {
return &schema.Resource{
Create: resourceGithubCustomPropertiesCreate,
Read: resourceGithubCustomPropertiesRead,
Update: resourceGithubCustomPropertiesUpdate,
Delete: resourceGithubCustomPropertiesDelete,
Importer: &schema.ResourceImporter{
State: resourceGithubCustomPropertiesImport,
},

CustomizeDiff: customdiff.Sequence(
customdiff.ComputedIf("slug", func(d *schema.ResourceDiff, meta interface{}) bool {
return d.HasChange("name")
}),
),

Schema: map[string]*schema.Schema{
"property_name": {
Type: schema.TypeString,
Required: true,
Description: "The name of the custom property",
},
"value_type": {
Type: schema.TypeString,
Optional: true,
Description: "The type of the custom property",
},
"required": {
Type: schema.TypeBool,
Optional: true,
Description: "Whether the custom property is required",
},
"default_value": {
Type: schema.TypeString,
Description: "The default value of the custom property",
Optional: true,
Computed: true,
},
Comment on lines +43 to +48
Copy link
Contributor

@felixlut felixlut Jul 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm working in the repository level equivalents of these resources (i.e., github_repository_custom_properties as opposed to github_organization_custom_properties) in #2316. The value of a custom property, and by extension the default_value of it as well, can either be a string or a list of strings depending on the type of custom property (multi_select --> list of strings). See Response schema in the API docs.

I've opted to represent list values as a comma separated string for simplicity (eg "option1,option2"), and would suggest doing so here as well. It wouldn't change the schema at all, only the logic when creating a custom property of type multi_select

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't use comma-separated strings for this! We have a use-case for explicitly using comma-separated strings as the values within our multi-select custom properties :-)

Copy link
Contributor

@felixlut felixlut Jul 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Back to the drawing board then 😅

I guess it's possible to use something other than a comma as a separator, but that has the same problem if someone actually has a usecase for it.

Another alternative is having two different attributes depending on the property type. valueList for multi_select and valueString for the rest. And have some logic for not allowing both to be set at the same time

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about an explicitly dynamic type with validation?

Copy link
Contributor

@felixlut felixlut Jul 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh cool, I wasn't aware that existed! I've restricted myself to the Terraform SDKv2 docs so far, as that is what this project is using (it upgraded from SDKv1 in Januari #1780). Dynamic values are only available if you use the Terraform Plugin Framework. Looking at the Terraform Framework, it looks like it has some major advantages compared to SDKv2.

With that said, going down that path requires buy in from the maintainers of the provider itself, since it's no longer a simple feature request. There seems to be support for using both the Framework and SDKv2 at the same time, but I don't think it's worth adding the framework just for using Dynamic Types for one resource. In that case it should be for the other benefits it brings. I think @kfcampbell is the right person to ask for opinions here

In either case, I think we should be able to figure out an interface for this that doesn't require structural changes to the provider itself,. I can see the following options:

  • String with some separator. Bad for the reasons mentioned previously in the thread
  • valueAsString and valueAsStringList
  • Treat all values as a list of strings. I.e., the value for a non-multi_select custom property is a list of strings of length 1, and mutli_select properties are a list of length x

My vote is probably on the latter. It would loke something like:

resource "github_repository_custom_properties" "test" {
  repository = "test-custom-properties"
  properties = {
    true_false    = ["true"]
    text          = ["asd123"]
    single_select = ["option1"]
    multi_select  = ["option2", "option3"]
  }
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bother, indeed. I've only been working with the newer framework recently.
Bear in mind that whilst they're likely not yet exposed through go-github, upstream custom properties already support additional types, including boolean and multi-select, so argument per-type likely doesn't scale well into the future.

Copy link
Contributor

@felixlut felixlut Jul 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mhm, looks like my idea of treating everything as lists of string isn't supported by SDKv2 either... hashicorp/terraform-plugin-sdk#62.

The issue above essentially says that the values of a Terraform map can only be primitive values, which includes TypeBool, TypeInt, TypeFloat, and TypeString. This means that multi_select custom property values, which are lists, has to be represented as a TypeString as long as SDKv2 is used.

Copy link
Contributor

@felixlut felixlut Jul 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, actually the above is only true as long as the schema is using a Map structure, which is how I've currently implemented the github_repository_custom_properties datasource since it reads all custom properties of the repo. Nothing stops the default_value in this PR to be either a TypeList/TypeString

Copy link
Contributor

@muru muru Aug 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not familiar with the internals of Terraform providers, but would it make sense to use a property block instead?

resource "github_repository_custom_properties" "test" {
  repository = "test-custom-properties"
  property {
    name  = "true_false"
    value = ["true"]
  }
  property {
    name  = "text"
    value = ["asd123"]
  }
  property {
    name  = "single_select"
    value = ["option1"]
  }
  property {
    name  = "multi_select"
    value = ["option2", "option3"]
  }
}

Copy link
Contributor

@isometry isometry Oct 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Internally, blocks are implemented more-or-less the same as object lists. Sense idiomatic to me.
Whether implemented with blocks or not, what about encoding the values as JSON to support the different property types. This is pretty common pattern, and HCL lends itself to easy marshal/unmarshal.

I would also personally vote for this being introduced as a block on the existing github_repository resource rather than introducing another (with associated per-resource billing models from most commercial state backends)

resource "github_repository" {
  ...

  custom_property {
    name  = "owner"
    value = jsonencode("bob")
  }

  custom_property {
    name  = "PoC"
    value = jsonencode(true)
  }

  custom_property {
    name  = "promotion_flow"
    value = jsonencode(["main","staging","production"])
  }
}

Edit: oops, this example really belongs on the other PR, but the point about JSON encoding the options holds :-)

"description": {
Description: "The description of the custom property",
Type: schema.TypeString,
Optional: true,
Computed: true,
},
"allowed_values": {
Description: "The allowed values of the custom property",
Type: schema.TypeList,
Optional: true,
Computed: true,
Elem: &schema.Schema{Type: schema.TypeString},
},
},
}
}

func resourceGithubCustomPropertiesCreate(d *schema.ResourceData, meta interface{}) error {
ctx := context.Background()
client := meta.(*Owner).v3client
ownerName := meta.(*Owner).name

propertyName := d.Get("property_name").(string)
valueType := d.Get("value_type").(string)
required := d.Get("required").(bool)
defaultValue := d.Get("default_value").(string)
description := d.Get("description").(string)
allowedValues := d.Get("allowed_values").([]interface{})
var allowedValuesString []string
for _, v := range allowedValues {
allowedValuesString = append(allowedValuesString, v.(string))
}

customProperty, _, err := client.Organizations.CreateOrUpdateCustomProperty(ctx, ownerName, d.Get("property_name").(string), &github.CustomProperty{
PropertyName: &propertyName,
ValueType: valueType,
Required: &required,
DefaultValue: &defaultValue,
Description: &description,
AllowedValues: allowedValuesString,
})
if err != nil {
return err
}

d.SetId(*customProperty.PropertyName)
return resourceGithubCustomPropertiesRead(d, meta)
}

func resourceGithubCustomPropertiesRead(d *schema.ResourceData, meta interface{}) error {
ctx := context.Background()
client := meta.(*Owner).v3client
ownerName := meta.(*Owner).name

customProperty, _, err := client.Organizations.GetCustomProperty(ctx, ownerName, d.Get("property_name").(string))
if err != nil {
return err
}

d.SetId(*customProperty.PropertyName)
d.Set("allowed_values", customProperty.AllowedValues)
d.Set("default_value", customProperty.DefaultValue)
d.Set("description", customProperty.Description)
d.Set("property_name", customProperty.PropertyName)
d.Set("required", customProperty.Required)
d.Set("value_type", customProperty.ValueType)

return nil
}

func resourceGithubCustomPropertiesUpdate(d *schema.ResourceData, meta interface{}) error {
if err := resourceGithubCustomPropertiesCreate(d, meta); err != nil {
return err
}
return resourceGithubTeamRead(d, meta)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you probably want to return the resourceGithubCustomPropertiesRead(d, meta) value, I guess this is a typo

}

func resourceGithubCustomPropertiesDelete(d *schema.ResourceData, meta interface{}) error {
client := meta.(*Owner).v3client
ownerName := meta.(*Owner).name

_, err := client.Organizations.RemoveCustomProperty(context.Background(), ownerName, d.Get("property_name").(string))
if err != nil {
return err
}

return nil
}

func resourceGithubCustomPropertiesImport(d *schema.ResourceData, meta interface{}) ([]*schema.ResourceData, error) {
// WIP
return []*schema.ResourceData{d}, nil
}