Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Include GrapheneUnauthorizedError in GrapheneTerminateRunsResultOrError #27925

Merged
merged 1 commit into from
Feb 19, 2025

Conversation

gibsondan
Copy link
Member

@gibsondan gibsondan commented Feb 19, 2025

Summary:
This mutation can fail a permissions check (in a somewhat odd edge case where the run doesn't exist at all...) so it should include this response type.

Test Plan:
BK

Copy link

github-actions bot commented Feb 19, 2025

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-ctr3w5i8z-elementl.vercel.app
https://allowunauthorizedresponse.core-storybook.dagster-docs.io

Built with commit 52f2c60.
This pull request is being automatically deployed with vercel-action

@gibsondan gibsondan force-pushed the allowunauthorizedresponse branch from 65719f9 to 267d214 Compare February 19, 2025 21:50
@@ -271,6 +272,14 @@ def test_cancel_runs_permission_failure(self, graphql_context: WorkspaceRequestC
in result.data["terminateRuns"]["terminateRunResults"][0]["message"]
)

def test_cancel_runs_non_existant_run(self, graphql_context: WorkspaceRequestContext):
Copy link

Choose a reason for hiding this comment

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

The method name contains a spelling error - existant should be existent. This matches standard English spelling and improves code readability.

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

@gibsondan gibsondan force-pushed the allowunauthorizedresponse branch 2 times, most recently from ddae0cb to 52f2c60 Compare February 19, 2025 21:51
@gibsondan gibsondan force-pushed the allowunauthorizedresponse branch from 52f2c60 to 4f0a130 Compare February 19, 2025 22:30
Summary:
This mutation can fail a permissions check (in a somewhat odd edge case where the run doesn't exist at all...) so it should include this response type.

Test Plan:
BK

> Insert changelog entry or delete this section.
@gibsondan gibsondan force-pushed the allowunauthorizedresponse branch from 4f0a130 to 54ca6ff Compare February 19, 2025 22:57
@gibsondan gibsondan merged commit 8f3f80c into master Feb 19, 2025
5 of 6 checks passed
@gibsondan gibsondan deleted the allowunauthorizedresponse branch February 19, 2025 23:24
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants