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

[docathon] pass tox for asset factories #23909

Closed

Conversation

cmpadden
Copy link
Contributor

@cmpadden cmpadden commented Aug 26, 2024

Summary & Motivation

  • minor tweaks to pass tests for asset factory snippets
  • skip snippet that has dependency on dagster-cloud

How I Tested These Changes

tox

Changelog [New | Bug | Docs]

NOCHANGELOG

Copy link

graphite-app bot commented Aug 26, 2024

Graphite Automations

"docs-beta - Assign Reviewers" took an action on this PR • (08/26/24)

2 reviewers were added to this PR based on Pedram Navid's automation.

Copy link

github-actions bot commented Aug 26, 2024

Deploy preview for dagster-docs-beta ready!

✅ Preview
https://dagster-docs-beta-diyrpv2lu-elementl.vercel.app
https://colton-asset-factories-stack.dagster.dagster-docs.io

Built with commit 3234f3d.
This pull request is being automatically deployed with vercel-action

@cmpadden cmpadden requested a review from petehunt August 26, 2024 14:36
@@ -14,7 +14,7 @@
"Operating System :: OS Independent",
],
packages=find_packages(exclude=["test"]),
install_requires=["dagster-cloud"],
install_requires=[],
Copy link
Contributor

Choose a reason for hiding this comment

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

wonder if tox.ini can point at another git repo so we can target latest master

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do think it's possible, but complicated slightly because it's in a private repo.

@@ -33,5 +37,5 @@ def load_etl_jobs_from_yaml(yaml_path: str) -> dg.Definitions:
return dg.Definitions.merge(*defs)


defs = load_etl_jobs_from_yaml("etl_jobs.yaml")
defs = load_etl_jobs_from_yaml(str(Path(__file__).parent / "etl_jobs.yaml"))
Copy link
Contributor

Choose a reason for hiding this comment

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

hoping we can get rid of this in all the examples and replace it with a chdir in the test case so the code samples are simpler

Copy link
Contributor Author

@cmpadden cmpadden Aug 26, 2024

Choose a reason for hiding this comment

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

I think this is a decent compromise, what do you think?

3234f3d

defs = load_etl_jobs_from_yaml(
    "docs_beta_snippets/guides/data-modeling/asset-factories/etl_jobs.yaml"
)

Copy link
Contributor

Choose a reason for hiding this comment

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

still think it's pretty noisy to have as a code sample in the guides

@cmpadden cmpadden closed this Aug 26, 2024
@cmpadden cmpadden deleted the colton/asset-factories-stack branch October 14, 2024 18:02
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.

2 participants