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.

Sign up for GitHub

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

Jump to bottom

asset check docs #16680

Merged
merged 2 commits into from
Sep 27, 2023
Merged

asset check docs #16680

merged 2 commits into from
Sep 27, 2023

Conversation

johannkm
Copy link
Contributor

@johannkm johannkm commented Sep 21, 2023 •
edited
Loading

@johannkm
Copy link
Contributor Author

johannkm commented Sep 21, 2023 •
edited
Loading

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@johannkm johannkm requested a review from sryza September 21, 2023 03:50
@johannkm johannkm force-pushed the johann/09-08-asset_check_docs branch from 2e7b74b to 41e5cc8 Compare September 21, 2023 14:14
@github-actions
Copy link

github-actions bot commented Sep 21, 2023 •
edited
Loading

Deploy preview for dagster-docs ready!

Preview available at https://dagster-docs-mlsw3ghmm-elementl.vercel.app
https://johann-09-08-asset-check-docs.dagster.dagster-docs.io

Direct link to changed pages:

@github-actions
Copy link

github-actions bot commented Sep 21, 2023 •
edited
Loading

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-xlkdf06mx-elementl.vercel.app
https://johann-09-08-asset-check-docs.core-storybook.dagster-docs.io

Built with commit 6066d78.
This pull request is being automatically deployed with vercel-action

sryza
sryza reviewed Sep 21, 2023
View reviewed changes
Copy link
Contributor

@sryza sryza left a comment

Choose a reason for hiding this comment

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

I think it's also worth including the example for how to make an asset check factory, though that doesn't need to be part of this PR.

docs/content/concepts/assets/asset-checks.mdx Outdated Show resolved Hide resolved
docs/content/concepts/assets/asset-checks.mdx Outdated Show resolved Hide resolved
docs/content/concepts/assets/asset-checks.mdx Outdated Show resolved Hide resolved
docs/content/concepts/assets/asset-checks.mdx Outdated Show resolved Hide resolved
docs/content/concepts/assets/asset-checks.mdx Outdated Show resolved Hide resolved
@johannkm johannkm force-pushed the johann/09-08-asset_check_docs branch 3 times, most recently from d59e780 to 6400c3e Compare September 26, 2023 01:02
@github-actions
Copy link

github-actions bot commented Sep 26, 2023 •
edited
Loading

Deploy preview for dagster-university ready!

✅ Preview
https://dagster-university-1uggsqlj9-elementl.vercel.app
https://johann-09-08-asset-check-docs.dagster-university.dagster-docs.io

Built with commit 6066d78.
This pull request is being automatically deployed with vercel-action

@github-actions
Copy link

Deploy preview for dagit-storybook ready!

✅ Preview
https://dagit-storybook-qifuglgqq-elementl.vercel.app
https://johann-09-08-asset-check-docs.components-storybook.dagster-docs.io

Built with commit 6400c3e.
This pull request is being automatically deployed with vercel-action

@johannkm johannkm force-pushed the johann/09-08-asset_check_docs branch from 6400c3e to 4d4ff73 Compare September 26, 2023 02:45
@johannkm johannkm requested a review from sryza September 26, 2023 02:59
@johannkm johannkm force-pushed the johann/09-08-asset_check_docs branch from 4d4ff73 to cf4d127 Compare September 26, 2023 03:26
@johannkm johannkm mentioned this pull request Sep 26, 2023
Merged
erinkcochran87
erinkcochran87 reviewed Sep 26, 2023
View reviewed changes
Copy link
Contributor

@erinkcochran87 erinkcochran87 left a comment

Choose a reason for hiding this comment

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

I did a quick pass on this and found some small things - back to you!

docs/content/concepts/assets/asset-checks.mdx Outdated Show resolved Hide resolved
docs/content/concepts/assets/asset-checks.mdx Show resolved Hide resolved
docs/content/concepts/assets/asset-checks.mdx Show resolved Hide resolved
docs/content/concepts/assets/asset-checks.mdx Outdated Show resolved Hide resolved
docs/content/concepts/assets/asset-checks.mdx Show resolved Hide resolved
docs/content/concepts/assets/asset-checks.mdx Outdated Show resolved Hide resolved
docs/content/concepts/assets/asset-checks.mdx Outdated

## Executing checks via the UI

Materializing a asset from the UI will also execute any checks that are defined for that asset. You can also execute checks without materializing the asset from the Checks tab of the asset’s detail page.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Materializing a asset from the UI will also execute any checks that are defined for that asset. You can also execute checks without materializing the asset from the Checks tab of the asset’s detail page.
Materializing an asset from the UI will also execute any checks that are defined for that asset. You can also execute checks without materializing the asset from the Checks tab of the asset’s detail page.

sryza
sryza reviewed Sep 26, 2023
View reviewed changes
Copy link
Contributor

@sryza sryza left a comment

Choose a reason for hiding this comment

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

I left a couple more small comments. After those and Erin's feedback, this LGTM!

docs/content/concepts/assets/asset-checks.mdx Outdated Show resolved Hide resolved
docs/content/concepts/assets/asset-checks.mdx Show resolved Hide resolved
docs/content/concepts/assets/asset-checks.mdx Show resolved Hide resolved
@johannkm johannkm force-pushed the johann/09-08-asset_check_docs branch from cf4d127 to 6066d78 Compare September 27, 2023 02:03
@johannkm
Copy link
Contributor Author

addressed comments

erinkcochran87
erinkcochran87 approved these changes Sep 27, 2023
View reviewed changes
Copy link
Contributor

@erinkcochran87 erinkcochran87 left a comment

Choose a reason for hiding this comment

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

🎉

@johannkm johannkm merged commit abc4a82 into master Sep 27, 2023
@johannkm johannkm deleted the johann/09-08-asset_check_docs branch September 27, 2023 17:21
PedramNavid pushed a commit that referenced this pull request Sep 28, 2023
@johannkm @erinkcochran87 @PedramNavid
asset check docs (#16680)
aa3e3f0 
First pass on docs, adapted from
https://www.notion.so/dagster/Proposal-Checks-APIs-and-docs-ffae0224509043dc84df700199f88315

---------

Co-authored-by: Erin Cochran <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erinkcochran87 erinkcochran87 erinkcochran87 approved these changes

@sryza sryza Awaiting requested review from sryza

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@johannkm @sryza @erinkcochran87