[BUG]: `data.github_organization.my_org.users[*].id` is unexpectedly a Node ID

omsmith commented 3 months ago

Expected Behavior

I would expect the id field of the user objects provided on data_source_github_organization to be the integer id of the user, similar to data_source_github_user: https://registry.terraform.io/providers/integrations/github/latest/docs/data-sources/user#id

Actual Behavior

The id field of the user object is the GraphQL Node Id, which is generally referred to as node_id, such as on data_source_github_user: https://registry.terraform.io/providers/integrations/github/latest/docs/data-sources/user#node_id and on data_source_github_organization itself: https://registry.terraform.io/providers/integrations/github/latest/docs/data-sources/organization#node_id

Terraform Version

Terraform v1.7.5 on linux_amd64 + provider registry.terraform.io/integrations/github v6.2.0

Affected Resource(s)

data_source_github_organization

Terraform Configuration Files

No response

Steps to Reproduce

No response

Debug Output

No response

Panic Output

No response

Code of Conduct

[X] I agree to follow this project's Code of Conduct

omsmith commented 3 months ago

Hello!

I was on the fence to open this as a bug vs a feature request. Everything is of course working, but the experience is inconsistent. Do not intend any offence by labelling it as a bug.

5 months back I attempted some refactoring of our terraform to remove what looked like unnecessary data "github_user" nodes once users was added to data_source_github_organizations. Came across the fact that id was actually the GraphQL node id and thus I couldn't use it for what we needed (other resources requiring the numeric id), so I put it down and forgot it about.

Yesterday I was revisiting some parts of that project and attempted the same refactoring, having forgotten about the limitation. Ended up with pretty well the same diff, and then had a good chuckle afterward.

If data_source_github_organization.users exposed the database id, it would allow us to remove some data "github_user" nodes for doing the id lookup, and hopefully provide a speed-up for our builds by doing so (plan/apply currently takes ~8 minutes for our largest org).

From the resources/data sources we've used, id usually refers to this database id whereas node_id is used to refer to the GraphQL node id, which is where this feels like an "unexpected behaviour / bug".

I'm not much of a gopher, nor spent any time developing terraform providers, but I did take a stab at a proposal in https://github.com/integrations/terraform-provider-github/pull/2206. Test weren't passing for me even on main though, so there's something off in my test environment I've yet to figure out.

Hope this issue and proposal makes some sense - happy to discuss.

kfcampbell commented 3 months ago

Hello! First of all, no offense taken; there's a whole lot wrong with the provider, and I'm always happy to talk about ways to make it better, especially if you're willing to open a PR to make it better, that's my favorite thing.

The ID thing I agree is annoying...my personal preference would be to expect ID to be the GitHub or database ID, and the GraphQL or Node ID to be node_id or similar in the schema. Today, we're all over the place: we have id in the schema referring to both the GraphQL ID and the database ID and we have database_id referring to the database ID.

The real issue that holds up a concentrated effort to change this is that breaking changes in Terraform are a pain; Hashicorp recommends we don't do more than one a year to minimize strain on consumers and we just recently did v6. I'm 👍 on the deprecation idea and your proposal seems reasonable, though realistically it'll be close to a year before that deprecation will be able to be carried out.

Test weren't passing for me even on main though, so there's something off in my test environment I've yet to figure out.

This isn't you; this is a symptom of the provider having bad test issues. On the main branch I see the following output for the relevant tests:

--- FAIL: TestAccGithubOrganizationDataSource (46.03s)
    --- FAIL: TestAccGithubOrganizationDataSource/queries_for_an_organization_without_error (8.82s)
        --- SKIP: TestAccGithubOrganizationDataSource/queries_for_an_organization_without_error/with_an_anonymous_account (0.00s)
        --- SKIP: TestAccGithubOrganizationDataSource/queries_for_an_organization_without_error/with_an_individual_account (0.00s)
        --- FAIL: TestAccGithubOrganizationDataSource/queries_for_an_organization_without_error/with_an_organization_account (8.82s)
    --- PASS: TestAccGithubOrganizationDataSource/queries_for_an_organization_with_archived_repos (37.20s)
        --- SKIP: TestAccGithubOrganizationDataSource/queries_for_an_organization_with_archived_repos/with_an_anonymous_account (0.00s)
        --- SKIP: TestAccGithubOrganizationDataSource/queries_for_an_organization_with_archived_repos/with_an_individual_account (0.00s)
        --- PASS: TestAccGithubOrganizationDataSource/queries_for_an_organization_with_archived_repos/with_an_organization_account (37.20s)
FAIL

and the test is failing due to the following error: data_source_github_organization_test.go:51: Step 1/1 error: Check failed: Check 2/26 error: data.github_organization.test: Attribute 'name' expected to be set.

All of this is to say: I'm receptive to your ideas and I think you're on the right track. If you want to push them further, I'd be happy to review. Adding additional integration test cases for your new fields (or dare I say, fixing the currently broken "happy path" test) would be much appreciated.

omsmith commented 3 months ago

:+1: Thanks for taking a look. I'll review the existing tests and see if they can't be improved before moving my existing PR out of draft state.

integrations / terraform-provider-github