feat: support repository level custom_property resource and custom_properties datasource #2316

felixlut · 2024-07-14T17:26:18Z

Contributes to #1956

Custom properties are defined on an organizational level, and then applied on a repository level. This PR aims to solve the latter of those two, while the former already has a PR for it in #2107.

Edit: Leaving the original comment below. Update can be found below in #2316 (review)

This is a work in progress (WIP) since the underlying sdk:s are lacking for the REST endpoints pertaining to the repository level custom properties:

Regular go-github lacks support for the multi_select custom property type, see Bug: Multi-select Custom Properties are not supported google/go-github#3198. There is a PR close to being merged for solving this feat!: Add support for multi-select Custom Properties google/go-github#3200
- The PR is merged, but no new release of go-github has been released since
As @kfcampbell mentioned here, the GitHub SDK team has created a SDK generated from GitHub's OpenAPI spec. I've added and used it to implement a github_repository_custom_properties datasource with it due to the lacking functionality of go-github in this area currently. I'm currently trying to get it to write the custom properties, which I've been unsuccessful in so far. See [BUG]: PATCH request for repos/OWNER/REPO/properties/values broken octokit/go-sdk#90

If there's any progress on either of these I'll update this PR

Before the change?

It was not possible to add a custom property to a repo before this change (or read them)

After the change?

It is now possible to add a custom property to a repo before this change (or read them)

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

…ository_custom_property instead

ViktorLindgren95 · 2024-09-01T10:41:20Z

@felixlut It seems that v64 google/go-github#3240 added support for Multi select finally. Only issue is that the terraform provider codebase is still at v63

felixlut · 2024-09-06T10:53:14Z

@felixlut It seems that v64 google/go-github#3240 added support for Multi select finally. Only issue is that the terraform provider codebase is still at v63

Seems like #2359 will deal with bumping the dep, so I'll revisit this once it's merged

felixlut · 2024-10-09T15:47:01Z

I started looking into this again over the weekend but hit a bottleneck again. I made a fix in google/go-github#3309, but that requires another bump of the go-github provider. Hopefully that is the last blocker before we can use that provider fully for this feature.

The reason I'd rather use that than the go-sdk is that the PR simply becomes huge, and anytime another PR is merged I have to resolve conflicts (or at least run the risk of it). Dealing with that on a long-living PR like this is pretty painful. Using the go-github will skip that pain 😅

…unction

stevehipwell · 2024-11-21T16:06:38Z

@felixlut given that go-github has been updated to v66 are you now unblocked?

felixlut · 2024-11-21T17:35:09Z

@felixlut given that go-github has been updated to v66 are you now unblocked?

Will take a look again in the coming week or so

felixlut

Aight, long time coming but I finally think this PR is in a reviewable state! I've made some comments on certain parts where I'm not 100% sure on the design, I'd love to get some opinions on those! Of the bat these 2 things are noteworthy as well:

I opted for a non-authoriative custom_propertY resource as opposed to an authoriative custom_propertIES resource. Reason being that fulfills the use-case I have. I think an authoriative resource should exist, as it has its uses similarily to github_repository_collaborators. Nothing stops later PR:s doing just that
- On a similar note I opted opted out of adding custom_property to github_repository, as proposed here for the same reason. Nothing stops future PR:s here either. Docs just needs to reflect that both shouldn't be used as the same time
For the data_source I opted for custom_propertIES over reading each property individually. The reason for this is that the API endpoint for reading custom properties on a repo returns ALL properties. Filtering that full list and returning just 1 property seemed redundant to me. With that said creating a custom_propertY data source should be trivial with a future PR 😄

Also, thanks @ViktorLindgren95 for juggling some ideas regarding this PR with me, much appreciated!

felixlut · 2024-11-23T22:35:37Z

github/resource_github_repository_custom_property.go

+			"property_value": {
+				Type:        schema.TypeSet,
+				MinItems:    1,
+				Required:    true,
+				Description: "Value of the custom property.",
+				Elem: &schema.Schema{
+					Type: schema.TypeString,
+				},
+				ForceNew: true,
+			},


The value of a custom property can be either null or string or array of strings because of the multi_select property type (docs). This is an interesting design choice in the API that poses some unintuitive challenges while interacting with it, namely how we design the Terraform resource for the property_value?

Have different fields for property_value_string and property_value_list depending on the input type?

Treat everything as a list of strings (and null as [])?

jsonencode:ed values?

This thread discusses it a bit more

I opted for the "treat everything as an array of strings" solution, as that is how GitHub have solved the issue in their other API:s that deal with custom properties (eg the Organization Rulesets API, hidden deep in .conditions.repository_property_and_ref_name.repository_property.include.property_values). Like with the other comments, I'm all ears for opinions here

felixlut · 2024-11-23T22:51:21Z

github/resource_github_repository_custom_property.go

+			"property_type": {
+				Type:             schema.TypeString,
+				Required:         true,
+				Description:      "Type of the custom property",
+				ForceNew:         true,
+				ValidateDiagFunc: toDiagFunc(validation.StringInSlice([]string{SINGLE_SELECT, MULTI_SELECT, STRING, TRUE_FALSE}, false), "property_type"),
+			},


One of the downsides of the limitations mentioned in the comment below (property_value can be null or string or array) is that we need to know which format the API expects when creating/updating the custom property. I.e., an array with 1 element can either represent a multi_select property, and the API then expects that we send the full array, or it could be a single_select (or any other type actually), where the API would then expect only a string in the call to it. Docs for the specific endpoint can be found here.

I had the following ideas as workarounds:

User supplies the custom_property_type in the resource. More responsibility on the user, but it works...

Given the name of the custom property, read it on an org level and thereby derive the value_type. Downside of this is that the user/App running Terraform needs "Custom properties" organization permissions (read)

One would imagine that there should be a corresponding repo-level endpoint where one could read the value_type, but I've not been able to find one. This is the closest I could find, but that just returns the name and value of an already set property, which is not enough for our problem

It's also possible to run a "greedy" attempt at creating/updating the custom property when the array is of length 1. First try running it with the single string, and if that fails try again with the array instead. The error message is pretty helpful here (see below). This feels incredibly hacky though...

I opted for the user having to fill in custom_property_type but I'd love to be convinced there is another way here

felixlut · 2024-11-23T22:55:43Z

github/data_source_github_repository_custom_properties_test.go

+
+func TestAccGithubRepositoryCustomPropertiesDataSource(t *testing.T) {
+
+	t.Skip("You need an org with custom properties already setup as described in the variables below") // TODO: at the time of writing org_custom_properties are not supported by this terraform provider, so cant be setup in the test itself for now


I've ran the tests locally and confirmed they work in my test org. Notably this should be updated once #2107 is merged (or another PR takes its place)

felixlut · 2024-11-23T23:03:36Z

github/resource_github_repository_custom_property_test.go

+
+func TestAccGithubRepositoryCustomProperty(t *testing.T) {
+
+	t.Skip("You need an org with custom properties already setup as described in the variables below") // TODO: at the time of writing org_custom_properties are not supported by this terraform provider, so cant be setup in the test itself for now


I've ran the tests locally and confirmed they work in my test org. Notably this should be updated once #2107 is merged (or another PR takes its place)

felixlut · 2024-11-24T10:20:28Z

github/data_source_github_repository_custom_properties.go

+				Description: "Name of the repository which the custom properties should be on.",
+			},
+			"property": {
+				Type:        schema.TypeSet,


The way I see most people using this resource is to:

Read the custom properties of a repo

For each specific property do something based on it's value. Eg, do X to the repo if the owner property equals Y, do Z to the repo if the environment property equals ...

This means that it should be as easy as possible to get the value of a specific property. IMO that would be best achieved by using a TypeMap. Unfortunately that is not supported by v2 of the terraform-plugin-sdk (see thread).

The choice is then between TypeList or TypeSet. Reading the docs the latter sounds more correct to me, as the order of the custom properties does not matter. We've seen issues regarding this in this provider in the past, eg in #1950, that should be avoided by using TypeSet.

It's worth noting that there are some other implications of using TypeSet though, namely that they are non-indexable. This common pattern will for example not work, and throw the error below:

data "github_repository_custom_properties" "test" { repository = "test-custom-properties" } locals { # Assuming the repo have a property called "string" set already test = data.github_repository_custom_properties.test.property[index(data.github_repository_custom_properties.test.property.*.property_name, "string")] }

That does not mean that it's impossible to fetch the value of a custom property though. Either of the two snippets below does work, and are IMO more readable, but I think I'd be amiss if I didn't mention this.

data "github_repository_custom_properties" "test" { repository = "test-custom-properties" } locals { # Get single custom property string_property = one([ for prop in data.github_repository_custom_properties.test.property: prop if prop.property_name == "string" ]) # Convert the output of the data_source to a map that can then be referenced by `local.property_map[<property_name>]` property_map = { for prop in data.github_repository_custom_properties.test.property: prop.property_name => prop.property_value } }

I personally lean towards using a TypeSet to avoid possible weird diffs, but I don't feel strongly about it. If anyone have any opinions here I'm all ears 😄

@felixlut have you considered also adding a github_repository_custom_property data source to access a singular property? It would allow for more direct access to properties at the cost of increased API usage.

I have, and nothing stops future PR:s for doing just that. It probably should exist, but you can't do everything in 1 PR 😅

The actual implementation would be to simply filter it after this call

ChaosCypher · 2025-01-09T14:23:48Z

🚀

hashildy · 2025-01-14T04:52:07Z

Any update on getting this PR merged? This would be a really useful enhancement to support custom properties!

felixlut · 2025-01-14T09:55:17Z

Aight, long time coming but I finally think this PR is in a reviewable state! I've made some comments on certain parts where I'm not 100% sure on the design, I'd love to get some opinions on those! Of the bat these 2 things are noteworthy as well:

I opted for a non-authoriative custom_propertY resource as opposed to an authoriative custom_propertIES resource. Reason being that fulfills the use-case I have. I think an authoriative resource should exist, as it has its uses similarily to github_repository_collaborators. Nothing stops later PR:s doing just that

On a similar note I opted opted out of adding custom_property to github_repository, as proposed here for the same reason. Nothing stops future PR:s here either. Docs just needs to reflect that both shouldn't be used as the same time

For the data_source I opted for custom_propertIES over reading each property individually. The reason for this is that the API endpoint for reading custom properties on a repo returns ALL properties. Filtering that full list and returning just 1 property seemed redundant to me. With that said creating a custom_propertY data source should be trivial with a future PR 😄

Also, thanks @ViktorLindgren95 for juggling some ideas regarding this PR with me, much appreciated!

@hashildy it hasn't moved since my last message ^^

It is ready for review though (or at least it was 2m ago).

hashildy · 2025-01-15T15:11:21Z

@stevehipwell - is this PR able to be reviewed/merged?

stevehipwell · 2025-01-15T17:14:11Z

@hashildy I'm just a contributor, I have 9 PRs that have been ready to review merge in the last 3 months that haven't had a single comment by a maintainer.

tayven-bigelow · 2025-01-15T22:34:08Z

@kfcampbell - Any chance of getting a review / approval of this PR?
Custom properties would be great to have support around in terraform.

felixlut added 9 commits July 12, 2024 15:27

add octokit sdk client

a7e562c

stash half working solution

af0245a

add repository custom properties data source

cdd112a

break out custom props parsing logic to its own function

f9633fb

use background ctx

5e0894d

format provider.go

2accb56

fix error msg to include repoName instead of its pointer

b3f44ca

use type switch instead of if else

98947a3

fix linting errors

2125cab

This was referenced Jul 15, 2024

🏳️ Custom properties resource & data #2107

Open

[FEAT]: Support GitHub repository custom properties #1956

Open

felixlut and others added 4 commits July 16, 2024 17:40

restructure datasource to take a property name and call it github_rep…

ec2755e

…ository_custom_property instead

formatting

1e88e7a

rename file to match datasource name

31f9a2b

Merge branch 'main' into repository-custom-properties

5ed47ba

felixlut added 4 commits October 5, 2024 21:18

remove go-sdk in favor of go-github

697c43c

implement data_source_github_repository_custom_property with go-github

9933170

implement resource_github_repository_custom_property

a7aefae

update descriptions

ce87c42

felixlut mentioned this pull request Oct 6, 2024

fix: Allow removing/unsetting repository custom property google/go-github#3309

Merged

felixlut mentioned this pull request Oct 9, 2024

[BUG]: PATCH request for repos/OWNER/REPO/properties/values broken octokit/go-sdk#90

Open

1 task

felixlut added 6 commits October 9, 2024 18:09

remove custom_property resource in favour of custom_propertIES one

6b76479

add custom_property resource to provider.go

d9ad9a9

formatting

7982a1a

add tests for repository_custom_property

7366a88

update description of test

fe5add1

add tests for each custom_property type

0dbd149

felixlut and others added 3 commits October 9, 2024 23:03

formatting

c72ce22

Merge branch 'main' into repository-custom-properties

bc93a88

breakout parsing custom_property_value as a string slice to its own f…

d298e07

…unction

felixlut mentioned this pull request Nov 14, 2024

feat: support team organization role assignment #2322

Open

4 tasks

felixlut and others added 2 commits November 21, 2024 17:21

Merge branch 'main' into repository-custom-properties

8eff6f4

bump go-github to v66 for new files

3deb3ed

felixlut and others added 6 commits November 22, 2024 11:45

add property_type as a required attribute

31ac7db

flip datasource to use typeList instead of typeSet

10bb6f0

refactor tests for data source to use property_type

1e3ba1d

Update data_source_github_repository_custom_properties.go

5be94bd

cleanup incorrect comments

d9974e6

remove old comment

add24a8

felixlut commented Nov 24, 2024

View reviewed changes

felixlut marked this pull request as ready for review November 24, 2024 10:29

felixlut changed the title ~~WIP: support repository_custom_properties resource and datasource~~ feat: support repository_custom_properties resource and datasource Nov 24, 2024

felixlut changed the title ~~feat: support repository_custom_properties resource and datasource~~ feat: support repository level custom_property resource and custom_properties datasource Nov 24, 2024

felixlut and others added 3 commits November 24, 2024 14:38

add docs

46c1846

fix typo

15b61e7

Merge branch 'main' into repository-custom-properties

2b169f1

Merge branch 'main' into repository-custom-properties

1b6bfaa

kfcampbell approved these changes Jan 17, 2025

View reviewed changes

kfcampbell merged commit 1ca7092 into integrations:main Jan 17, 2025
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support repository level custom_property resource and custom_properties datasource #2316

feat: support repository level custom_property resource and custom_properties datasource #2316

felixlut commented Jul 14, 2024 •

edited

Loading

ViktorLindgren95 commented Sep 1, 2024 •

edited

Loading

felixlut commented Sep 6, 2024

felixlut commented Oct 9, 2024 •

edited

Loading

stevehipwell commented Nov 21, 2024

felixlut commented Nov 21, 2024

felixlut left a comment •

edited

Loading

felixlut Nov 23, 2024

felixlut Nov 23, 2024

felixlut Nov 23, 2024

felixlut Nov 23, 2024

felixlut Nov 24, 2024 •

edited

Loading

stevehipwell Nov 25, 2024

felixlut Nov 26, 2024 •

edited

Loading

ChaosCypher commented Jan 9, 2025

hashildy commented Jan 14, 2025

felixlut commented Jan 14, 2025

hashildy commented Jan 15, 2025

stevehipwell commented Jan 15, 2025

tayven-bigelow commented Jan 15, 2025


		func TestAccGithubRepositoryCustomPropertiesDataSource(t *testing.T) {

		t.Skip("You need an org with custom properties already setup as described in the variables below") // TODO: at the time of writing org_custom_properties are not supported by this terraform provider, so cant be setup in the test itself for now

feat: support repository level custom_property resource and custom_properties datasource #2316

feat: support repository level custom_property resource and custom_properties datasource #2316

Conversation

felixlut commented Jul 14, 2024 • edited Loading

Before the change?

After the change?

Pull request checklist

Does this introduce a breaking change?

ViktorLindgren95 commented Sep 1, 2024 • edited Loading

felixlut commented Sep 6, 2024

felixlut commented Oct 9, 2024 • edited Loading

stevehipwell commented Nov 21, 2024

felixlut commented Nov 21, 2024

felixlut left a comment • edited Loading

Choose a reason for hiding this comment

felixlut Nov 23, 2024

Choose a reason for hiding this comment

felixlut Nov 23, 2024

Choose a reason for hiding this comment

felixlut Nov 23, 2024

Choose a reason for hiding this comment

felixlut Nov 23, 2024

Choose a reason for hiding this comment

felixlut Nov 24, 2024 • edited Loading

Choose a reason for hiding this comment

stevehipwell Nov 25, 2024

Choose a reason for hiding this comment

felixlut Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

ChaosCypher commented Jan 9, 2025

hashildy commented Jan 14, 2025

felixlut commented Jan 14, 2025

hashildy commented Jan 15, 2025

stevehipwell commented Jan 15, 2025

tayven-bigelow commented Jan 15, 2025

felixlut commented Jul 14, 2024 •

edited

Loading

ViktorLindgren95 commented Sep 1, 2024 •

edited

Loading

felixlut commented Oct 9, 2024 •

edited

Loading

felixlut left a comment •

edited

Loading

felixlut Nov 24, 2024 •

edited

Loading

felixlut Nov 26, 2024 •

edited

Loading