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

Allow json serialized strings to be passed in as run config to graphql calls #6071

Merged
merged 1 commit into from Jan 7, 2022

Conversation

gibsondan
Copy link
Member

Summary:
In some languages / graphql clients it is a pain to set up a scalar that is a JSON object, and it's easier for clients to serialize up the run config as a string and upload that (even if it ends up double-encoded since the transport is also JSON-encoded).

This PR adds that as an option.

Summary

Test Plan

Checklist

  • My change requires a change to the documentation and I have updated the documentation accordingly.
  • I have added tests to cover my changes.

@vercel
Copy link

vercel bot commented Dec 30, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

dagit-storybook – ./js_modules/dagit/packages/core

🔍 Inspect: https://vercel.com/elementl/dagit-storybook/E7bZPXZ5cvu9WJBRQg6T5zB5mF1E
✅ Preview: Canceled

[Deployment for e096ad5 canceled]

dagster – ./docs/next

🔍 Inspect: https://vercel.com/elementl/dagster/3ZZvcySRFKU9QTr5pK6G2ysbhAT1
✅ Preview: Canceled

[Deployment for e096ad5 canceled]

Copy link
Member

@alangenfeld alangenfeld left a comment

Choose a reason for hiding this comment

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

I believe @smackesey was going to add support for uploading as yaml as well - if were going to be flexible on a single input arg vs having separate args for json, should this arg allow yaml as well?

Comment on lines 150 to 151
except JSONDecodeError:
return run_config
Copy link
Member

Choose a reason for hiding this comment

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

hmmm

Copy link
Member

Choose a reason for hiding this comment

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

can we at least put this under test to ensure the error is reasonable. If its not might be better to throw here that it was a string and not parseable

@gibsondan
Copy link
Member Author

I'd be fine with that. Did you have an idea in mind for how the backend would distinguish between whether it's JSON or YAML? (Attempting to parse it in each format is of course one option)

@alangenfeld
Copy link
Member

I'd be fine with that. Did you have an idea in mind for how the backend would distinguish between whether it's JSON or YAML? (Attempting to parse it in each format is of course one option)

Since we are already doing the try parse except shenanigans, seems fitting.

Doesn't need to be in this PR, just seemed like a good place to discuss it since we are adding flexibility to the arg.

Comment on lines 150 to 151
except JSONDecodeError:
return run_config
Copy link
Member

Choose a reason for hiding this comment

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

can we at least put this under test to ensure the error is reasonable. If its not might be better to throw here that it was a string and not parseable

@vercel vercel bot temporarily deployed to Preview – dagit-storybook January 7, 2022 20:48 Inactive
@vercel vercel bot temporarily deployed to Preview – dagster January 7, 2022 20:48 Inactive
…l calls

Summary:
In some languages / graphql clients it is a pain to set up a scalar that is a JSON object, and it's easier for clients to serialize up the run config as a string and upload that (even if it ends up double-encoded since the transport is also JSON-encoded).

This PR adds that as an option.
@vercel vercel bot temporarily deployed to Preview – dagster January 7, 2022 20:55 Inactive
@vercel vercel bot temporarily deployed to Preview – dagit-storybook January 7, 2022 20:55 Inactive
@gibsondan gibsondan merged commit 523024c into master Jan 7, 2022
@gibsondan gibsondan deleted the runconfigdata branch January 7, 2022 21:10
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.

None yet

2 participants