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

Fix handling of GHE without merge queue support #926

Closed
wants to merge 1 commit into from

Conversation

alex-statsig
Copy link
Contributor

@alex-statsig alex-statsig commented Jul 25, 2024

Fix handling of GHE without merge queue support

Summary:
#830 and #906 reported sapling not working for old Github Enterprise versions since their graphql schema doesnt include merge queue.

This adds a query to detect if merge queue is supported in the graphql version. Then, it will issue a different "YourPullRequestsQuery" depending on if merge queue is supported.

I considered a few other approaches:

  • Just detect the error message and retry without merge queue field - a little gross to extract from the error, felt much nicer to explicitly detect the support once
  • Use "@include" instead of an entirely separate copy+pasted query - this didn't seem to count as a valid query still via graphiql at least (unsure if the query client here would strip out the unused field though)

Test Plan:
Haven't had a chance to test yet, will try and validate that both versions work and that the detection logic works, but wanted to put up the PR first at least so someone could potentially take over if needed

Summary:
facebook#830 and facebook#906 reported sapling not working for old Github Enterprise versions since their graphql schema doesnt include merge queue.

This adds a query to detect if merge queue is supported in the graphql version. Then, it will issue a different "YourPullRequestsQuery" depending on if merge queue is supported.

I considered a few other approaches:
- Just detect the error message and retry without merge queue field - a little gross to extract from the error
- Use "@include" instead of an entirely separate query - didn't seem to count as a valid query still via graphiql at least (unsure if the query client here would strip out the unused field though)

Test Plan:
Haven't had a chance to test yet, will try and validate that both versions work and that the detection logic works.
@facebook-github-bot
Copy link
Contributor

@facebook-github-bot has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@luyiyang16
Copy link

@muirdm @zzl0 @kavehahmadi60
Could any of you take a look and help merging it if possible. Thanks!

@evangrayk
Copy link
Contributor

This looks good to me and works in my testing, but I don't have a github enterprise repo without merge support. Would love help validating that flow works as expected.

@facebook-github-bot
Copy link
Contributor

This pull request has been merged in 4aa5697.

@evangrayk
Copy link
Contributor

evangrayk commented Jul 25, 2024

This should be available now in sapling-scm VS Code extension version 0.1.54, please let me know if this works for folks using GHE!

@luyiyang16
Copy link

This should be available now in sapling-scm VS Code extension version 0.1.54, please let me know if this works for folks using GHE!

I am not using VS Code extension but using isl. Is there a way to patch this to ISL if I don't want to run ISL locally

@evangrayk
Copy link
Contributor

I am not using VS Code extension but using isl. Is there a way to patch this to ISL if I don't want to run ISL locally

I think you either need to build from source or wait for the next Sapling release, which is hopefully coming soon

@luyiyang16
Copy link

I am not using VS Code extension but using isl. Is there a way to patch this to ISL if I don't want to run ISL locally

I think you either need to build from source or wait for the next Sapling release, which is hopefully coming soon

I tried it locally build from source. Although the error is gone, I am still seeing trouble integrating with GHE - a lot of spinners. Not sure if it is because how I patched it locally doesn't work.

Do you know who should reach out for the next release?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants