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

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

Open
1 task done
omsmith opened this issue Mar 22, 2024 · 3 comments · May be fixed by #2206
Open
1 task done

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

omsmith opened this issue Mar 22, 2024 · 3 comments · May be fixed by #2206
Labels
Status: Up for grabs Issues that are ready to be worked on by anyone Type: Bug Something isn't working as documented

Comments

@omsmith
Copy link

omsmith commented Mar 22, 2024

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

  • I agree to follow this project's Code of Conduct
@omsmith omsmith added Status: Triage This is being looked at and prioritized Type: Bug Something isn't working as documented labels Mar 22, 2024
@omsmith omsmith linked a pull request Mar 22, 2024 that will close this issue
4 tasks
@omsmith
Copy link
Author

omsmith commented Mar 22, 2024

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 #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 kfcampbell added Status: Up for grabs Issues that are ready to be worked on by anyone and removed Status: Triage This is being looked at and prioritized labels Apr 1, 2024
@kfcampbell
Copy link
Member

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
Copy link
Author

omsmith commented Apr 1, 2024

👍 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Up for grabs Issues that are ready to be worked on by anyone Type: Bug Something isn't working as documented
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants