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

[structured config] Migrate resources from project-fully-featured to struct config resources #11785

Merged
merged 7 commits into from
Feb 16, 2023
Merged

[structured config] Migrate resources from project-fully-featured to struct config resources #11785

merged 7 commits into from
Feb 16, 2023

Conversation

benpankow
Copy link
Member

@benpankow benpankow commented Jan 18, 2023 •
edited
Loading

Summary

Migrates the experimental copy of project-fully-featured to use struct config resources instead of traditional function-based resources.

Test Plan

Existing unit tests.

@vercel
Copy link

vercel bot commented Jan 18, 2023 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Ignored Deployments
Name Status Preview Comments Updated
dagit-storybook ⬜️ Ignored (Inspect) Feb 16, 2023 at 6:33PM (UTC)
dagster ⬜️ Ignored (Inspect) Visit Preview Feb 16, 2023 at 6:33PM (UTC)

This was referenced Jan 18, 2023
Merged
Merged
@benpankow
Copy link
Member Author

benpankow commented Jan 18, 2023 •
edited
Loading

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@benpankow benpankow force-pushed the benpankow/migrate-proj-fully-featured branch from 581494c to 3736a4c Compare January 18, 2023 23:34
@benpankow benpankow mentioned this pull request Jan 19, 2023
Closed
@benpankow benpankow changed the base branch from benpankow/wip-partial-resources to benpankow/allow-nesting-non-structured-resources January 19, 2023 01:04
@benpankow benpankow force-pushed the benpankow/migrate-proj-fully-featured branch from c50fdb0 to b9c0994 Compare January 19, 2023 01:04
@benpankow benpankow force-pushed the benpankow/migrate-proj-fully-featured branch from b9c0994 to 2f6aff2 Compare January 19, 2023 01:09
@benpankow benpankow force-pushed the benpankow/allow-nesting-non-structured-resources branch from 452c979 to 20ca644 Compare January 20, 2023 18:05
@benpankow benpankow force-pushed the benpankow/migrate-proj-fully-featured branch from 2f6aff2 to cb3d3ca Compare January 20, 2023 18:05
@benpankow benpankow force-pushed the benpankow/allow-nesting-non-structured-resources branch from 20ca644 to 0f20a66 Compare January 20, 2023 18:27
@benpankow benpankow force-pushed the benpankow/migrate-proj-fully-featured branch from cb3d3ca to 7576da7 Compare January 20, 2023 18:27
schrockn
schrockn previously requested changes Jan 20, 2023
View reviewed changes
Copy link
Member

@schrockn schrockn 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 this feels pretty good overall. Can we make s3_bucket a bare string?

...esources/project_fully_featured_v2_resources/resources/common_bucket_s3_pickle_io_manager.py Outdated Show resolved Hide resolved
@benpankow benpankow force-pushed the benpankow/allow-nesting-non-structured-resources branch from 0f20a66 to f9cc06d Compare January 20, 2023 23:07
@benpankow benpankow force-pushed the benpankow/migrate-proj-fully-featured branch from 8359b0e to f70d71c Compare January 20, 2023 23:08
@benpankow benpankow force-pushed the benpankow/allow-nesting-non-structured-resources branch from f9cc06d to d604137 Compare January 20, 2023 23:15
@benpankow benpankow force-pushed the benpankow/migrate-proj-fully-featured branch from f70d71c to f465972 Compare January 20, 2023 23:15
schrockn
schrockn approved these changes Feb 4, 2023
View reviewed changes
Copy link
Member

@schrockn schrockn left a comment

Choose a reason for hiding this comment

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

This is great

...ect_fully_featured_v2_resources/project_fully_featured_v2_resources/resources/hn_resource.py
@@ -39,19 +41,20 @@ def min_item_id(self) -> int:


class HNSnapshotClient(HNClient):
def __init__(self):
@cached_method
Copy link
Member

Choose a reason for hiding this comment

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

Seems a bit odd to auto-cache this

@benpankow benpankow force-pushed the benpankow/wip-partial-resources branch from 7727f39 to 8b14151 Compare February 6, 2023 20:23
@benpankow benpankow force-pushed the benpankow/migrate-proj-fully-featured branch from 40096e8 to 292bf77 Compare February 6, 2023 20:23
@benpankow benpankow force-pushed the benpankow/wip-partial-resources branch from 8b14151 to eba049e Compare February 7, 2023 23:51
@benpankow benpankow force-pushed the benpankow/migrate-proj-fully-featured branch from 292bf77 to 5978010 Compare February 7, 2023 23:52
@benpankow benpankow force-pushed the benpankow/wip-partial-resources branch from eba049e to adf2e41 Compare February 8, 2023 00:23
@benpankow benpankow force-pushed the benpankow/migrate-proj-fully-featured branch from 5978010 to cd28e9d Compare February 8, 2023 00:23
@benpankow benpankow mentioned this pull request Feb 9, 2023
Merged
@benpankow benpankow force-pushed the benpankow/wip-partial-resources branch 5 times, most recently from 5ef268e to 45d2669 Compare February 14, 2023 21:33
Base automatically changed from benpankow/wip-partial-resources to master February 15, 2023 21:33
@benpankow benpankow mentioned this pull request Feb 15, 2023
Merged
@benpankow benpankow force-pushed the benpankow/migrate-proj-fully-featured branch from cd28e9d to 7660b62 Compare February 16, 2023 18:09
@benpankow benpankow force-pushed the benpankow/migrate-proj-fully-featured branch from 7660b62 to 17a9b34 Compare February 16, 2023 18:33
@benpankow benpankow merged commit b592861 into master Feb 16, 2023
@benpankow benpankow deleted the benpankow/migrate-proj-fully-featured branch February 16, 2023 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@schrockn schrockn schrockn approved these changes

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

Successfully merging this pull request may close these issues.
2 participants
@benpankow @schrockn