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

Add GraphQL Queries for Managing the IP Allow List (and Other Small Usability Fixes) #757

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

jusuchin85
Copy link
Contributor

@jusuchin85 jusuchin85 commented Nov 19, 2024

This pull request introduces several new GraphQL queries and mutations to manage IP allow lists for organizations and enterprise accounts. The changes include adding, enabling, disabling, and removing IP allow list entries, as well as renaming some existing query files for better clarity.

IP Allow List Management:

  • Added a query to retrieve current IP allow list settings for an enterprise, including entries and enabled settings (graphql/queries/enterprise-get-ip-allow-list.graphql).
  • Added a mutation to add an IP address to the IP allow list for both organizations and enterprise accounts (graphql/queries/ip-allow-list-add-ip.graphql).
  • Added a mutation to disable the IP allow list feature for GitHub Apps only (graphql/queries/ip-allow-list-disable-github-apps-only.graphql).
  • Added a mutation to disable the IP allow list feature for IP addresses only (graphql/queries/ip-allow-list-disable-ip-address-only.graphql).
  • Added a mutation to enable the IP allow list feature for both IP addresses and GitHub Apps (graphql/queries/ip-allow-list-enable.graphql).

SAML Identity Management:

  • Renamed the query file for listing SSO user identities and updated the query to use a fixed enterprise slug (graphql/queries/enterprise-saml-identities.graphql).

General Query Updates:

  • Renamed and updated the query for adding comments to issues to use a different source for ISSUE_ID (graphql/queries/issue-add-comment.graphql).
  • Renamed and updated the query for fetching branches and commits by repository to use fixed organization and repository names (graphql/queries/org-branches-and-commits-by-repository.graphql).
  • Added a query to retrieve IP allow list settings for an organization (graphql/queries/org-get-ip-allow-list.graphql).
  • Updated the query for fetching members by team to use fixed organization and team names (graphql/queries/org-members-by-team.graphql).

Renaming for Clarity:

Adding some GraphQL queries for managing the IP allow list feature in
github.com. These queries include:

- a query for getting the IP allow list configuration for an enterprise.
- a query for getting the IP allow list configuration for an
  organization.
- a query for adding an IP address to an IP allow list.
- a query for removing an IP address from an IP allow list.
- a query for enabling an IP allow list.
- a query for disabling an IP allow list.
The previous file names were a bit disorganised (some had numbers in the
beginning, while some had the scope between the file name).

This commit addresses this to ensure that consumers are able to identify
which GraphQL file to look for based on their needs.
The existing variables are too specific to the GitHub org. This commit
addresses this in the following files:

- org-repos-fragment-directive-2.graphql
- org-repos-fragment-directive.graphql

---

In addition, the following files have references to variable usage, but
was not explicitly declared anywhere:

- enterprise-saml-identities.graphql
- org-branches-and-commits-by-repository.graphql
- org-members-by-team.graphql
- org-pr-merged-info-by-repository.graphql
- repo-get-all-branches.graphql
- repos-get-last-issue-comment.graphql

I've updated the above files to just use simple strings to replace prior
to using the queries in them.
I noticed from another PR that we are using `ORG_NAME` for generic
organization variables. This commit updates all the queries to use
`ORG_NAME` instead of `ORGANIZATION_SLUG` to be consistent with the rest
of the codebase.
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.

I've tested all the queries and mutations and they work and look great, thanks for your effort cleaning up as well.

One thing I want to note is the clientMutationId: "true", this input is "always added, but it’s not passed to the resolve method. The value is re-inserted to the response. (It’s for client libraries to manage optimistic updates.)" per https://graphql-ruby.org/api-doc/1.8.13/GraphQL/Schema/RelayClassicMutation e.g.

That means to me that it is not necessary to pass (as I've tested as well for at least one mutation), but it also might be confusing to set this to "true" as this is not a boolean. Maybe we should either remove it, or replace it with a placeholder like "CLIENT_MUTATION_ID" to make sure this is understood as it should be?

Otherwise I am happy to 👍🏻

This variable is always added, so it is not necessary to set it
manually.

Reference doc: https://graphql-ruby.org/api-doc/1.8.13/GraphQL/Schema/RelayClassicMutation

Thanks to @sn2b for pointing this out!
Added new queries to separately enable and disable IP allow lists for
GitHub Apps only and IP addresses only.
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