🏳️ Custom properties resource & data

[alileza]

Resolves #1956

data.github_organization_custom_properties.prop_team: Reading...
data.github_organization_custom_properties.prop_team: Read complete after 0s [id=org-custom-properties]

Changes to Outputs:
  + team = {
      + allowed_values = [
          + "v1-alpha"
          + "v2-alpha"
          + "v2-beta"
        ]
      + default_value  = ""
      + description    = <<-EOT
            GitHub action for policy check based on conftest
            
            https://github.com/alileza/gha-team
        EOT
      + id             = "prop_team"
      + property_name  = "version"
      + required       = false
      + value_type     = "single_select"
    }

You can apply this plan to save these new output values to the Terraform state, without changing any real infrastructure.

---

Before the change?

After the change?

Pull request checklist

• Tests for the changes have been added (for bug fixes / features)
• Docs have been reviewed and added / updated if needed (for bug fixes / features)

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

• Yes
• No

---

[Simon-Boyer]

If I'm ready this correctly, this is only adding support to create/read custom properties definitions, would it also be possible to add support for defining custom properties values (by repo)?

[roeishuster]

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

[Simon-Boyer]

If I'm ready this correctly, this is only adding support to create/read custom properties definitions, would it also be possible to add support for defining custom properties values (by repo)?

I've looked a bit into it, and it should be possible once PR #2188 is merged to update go-github, since there was bug relating to that api that was fixed. I'm likely to try to build a working version of that feature on my side, i'll follow up there once it's done.

[Pirikara]

Looks like go-github has been updated to v62 with PR #2304 merged in place of PR #2187 !

[felixlut]

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

[isometry]

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 :-)

[felixlut]

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

[isometry]

What about an explicitly dynamic type with validation?

[felixlut]

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
  • Note - Add support for maps with non-primitive types (rich / object map support) hashicorp/terraform-plugin-sdk#62. This limitation makes it so we cannot create an authoritative github_repository_custom_properties resource, i.e., a resource that handles ALL custom_properties for a repo in one go, since that would require a Map of Strings to Lists of Strings. A github_repository_custom_property resource that can handle 1 single attribute is fine though, since we don't have to use a Map for the custom property names.

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"]
  }
}

[isometry]

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.

[felixlut]

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.

[felixlut]

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

[muru]

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"]
  }
}

[isometry]

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 :-)

[stevehipwell]

Given that #2316 has been merged and released this should really be a priority. @alileza are you still actively working on this?

[graham1228]

➕ 1️⃣ on getting this merged. Would love to migrate our bash scripts to TF, but need this functionality.

[nickfloyd]

CI is failing because it's looking for an older version of go-github @ v57 whereas the version in branch is v66.

[nickfloyd]

Additionally, we will need to merge in docs for this change after this PR get's merged.

[nickfloyd]

Approving, with the need for a fast follow up on getting linting working as well as docs merged in.