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

Updating the GraphQL queries for enterprise and org member data (saml identities, scim identities, emails for EMUs) #683

Merged
merged 25 commits into from
Oct 12, 2023

Conversation

stacycarter
Copy link
Contributor

@stacycarter stacycarter commented Oct 3, 2023

This PR is to update various GraphQL queries for listing enterprise or organization member details such as SAML identities, SCIM identities, enterprise member email addresses (emails for EMU enterprises specifically), and other details.

The query that should be used from this list and the output that is returned will depend on both the type of GitHub Enterprise Cloud enterprise (EMU or non-EMU), and the type of authentication/SCIM configuration that is in place.

List of queries being added/updated:

List of query files being removed (for cleanup and to avoid duplication since the queries above should take are of it)

…e-organization-scim-linked-identities.graphql
@github github deleted a comment from sunami1 Oct 4, 2023
@stacycarter stacycarter marked this pull request as ready for review October 4, 2023 14:45
@stacycarter stacycarter changed the title Updating the GraphQL SAML query platform samples Updating the GraphQL queries for enterprise and org member data (saml identities, scim identities, emails for EMUs) Oct 4, 2023
@sn2b
Copy link
Contributor

sn2b commented Oct 12, 2023

Hi @stacycarter 👋🏻 The queries all seem to work fine. I just have a couple of "cleanup" points. I don't know if there is a best practice or skeleton to use for this repo, but I'd like to point out a couple things:

  • Some queries start with query { while others don't. E.g. the explorer seems to fail without the query present.
  • Including or not including pagination. Some queries have the pageInfo but don't use it and some don't have it at all. For "simple" queries, meaning ones that only have one first, we could think about just adding in pagination. Alternatively I might be inclined to leave it out entirely.
  • Similarly for the slug for example, some queries use the variables externally and some pass it straight through. I personally think it would be good to have a uniform way of approaching this, tbh either way is fine in my opinion for these little test scripts.

Let me know if you want to chat about this. I am also open to hear other opinions. Like I said, the queries work and I'd be happy to approve this either way, I just thought I'd mention it.

Copy link
Contributor

@sn2b sn2b left a comment

Choose a reason for hiding this comment

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

Just saw your comment in the #support-squad-ecosystem Slack thread about adding pagination later! Sorry I missed that earlier.

@stacycarter
Copy link
Contributor Author

Thanks for the review and the helpful comments @sn2b! ❤️ I completely agree - I think the queries need to be cleaned up and made more consistent. I'm thinking what we could do is merge the changes we have in this PR, and then as time allows, a new PR could be opened to make the improvements that you mentioned above.

@stacycarter stacycarter merged commit 5c5d573 into master Oct 12, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants