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

Remove merge queue from query is feature is disabled #370

Merged
merged 1 commit into from
Dec 7, 2023

Conversation

ejoffe
Copy link
Owner

@ejoffe ejoffe commented Dec 7, 2023

fixes #356

commit-id:0219e1da

Copy link

@pleicht pleicht left a comment

Choose a reason for hiding this comment

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

Thanks a bunch for making this change! I think the condition for when to add the MergeQueue to the operation string needs to be inverted

@@ -17,7 +18,7 @@ query PullRequests(
repository {
id
}
mergeQueueEntry {
mergeQueueEntry @skip(if: $use_merge_queue) {
Copy link

Choose a reason for hiding this comment

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

Same here with inverting condition

Suggested change
mergeQueueEntry @skip(if: $use_merge_queue) {
mergeQueueEntry @skip(if: !$use_merge_queue) {

@@ -91,7 +92,7 @@ func (c *gqlclient) PullRequests(ctx context.Context,
repository {
id
}
mergeQueueEntry {
mergeQueueEntry @skip(if: $use_merge_queue) {
Copy link

Choose a reason for hiding this comment

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

I think the condition needs to be inverted

Suggested change
mergeQueueEntry @skip(if: $use_merge_queue) {
mergeQueueEntry @skip(if: !$use_merge_queue) {

Copy link

Choose a reason for hiding this comment

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

This file is probably generated from the above. Still completely new to Go

@ejoffe
Copy link
Owner Author

ejoffe commented Dec 7, 2023

Ah, you are correct. My bad. Will flip it.

@pleicht
Copy link

pleicht commented Dec 7, 2023

Looks right to me. The condition is to skip this field if use_merge_queue == true.

I tested it locally when building this branch and I get the error for merge queue. I think we only want to generate the MergeQueue section if use_merge_queue is true, if false we exclude

@ejoffe
Copy link
Owner Author

ejoffe commented Dec 7, 2023

Should be correct now. Good catch.

@pleicht
Copy link

pleicht commented Dec 7, 2023

Not sure if I'm doing something wrong with my local build, but I'm getting:

panic: query PullRequests.viewer.pullRequests.nodes.mergeQueueEntry: Field 'mergeQueueEntry' doesn't exist on type 'PullRequest'
query PullRequests: Variable $no_merge_queue is declared by PullRequests but not used

@ejoffe
Copy link
Owner Author

ejoffe commented Dec 7, 2023

I guess the skip directive doesn't work in this case, will try another approach.

@pleicht
Copy link

pleicht commented Dec 7, 2023

I guess the skip directive doesn't work in this case, will try another approach.

👍, thanks for the help and spr in general, I really enjoy using it!

@ejoffe
Copy link
Owner Author

ejoffe commented Dec 7, 2023

Much more involved fix pushed. I think this one should work for you. Let me know.

Copy link

@pleicht pleicht left a comment

Choose a reason for hiding this comment

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

Tested and confirmed it is working for me. Thanks!

@ejoffe ejoffe merged commit f1139b3 into master Dec 7, 2023
2 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.

Latest version incompatible with older GitHub Enterprise Server usages.
2 participants