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

AIP-47 - Migrate s3 DAGs to new design #22438 #24240

Closed
wants to merge 4 commits into from

Conversation

chethanuk
Copy link
Contributor

No description provided.

@josh-fell
Copy link
Contributor

CC @ferruzzi @vincbeck @o-nikolas I know all of you have been doing a ton of work standardizing the Amazon provider including the example DAGs.

@vincbeck
Copy link
Contributor

vincbeck commented Jun 6, 2022

Whao! Thanks for doing that @chethanuk-plutoflume . One general comment would be, we would like each sample dag to be self contained, meaning each sample dag creates and deletes all the resources/infrastructure it is using. One example would be in example_dynamodb_to_s3.py to create the S3 bucket and Dynamo table as setup and remove them as teardown. A good example is #24058

Copy link
Contributor

@o-nikolas o-nikolas left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution! Vincent's comment above is the most important feedback (also be sure to use the env_id in the names of the resources you create during setup so that they're unique).

Also another small nit is that we like to annotate the final task chain with which tasks are test setup, test body and test teardown. The Athena example shows this as a good example.

@chethanuk
Copy link
Contributor Author

create the S3 bucket and Dynamo table as setup and remove them as teardown

Let's create separate PR, it might be out of scope for this PR? Since we just focusing on AIP-47?

@chethanuk chethanuk closed this Jul 20, 2022
@chethanuk chethanuk deleted the aip-47-s3 branch July 20, 2022 21:35
@ferruzzi
Copy link
Contributor

Let's create separate PR, it might be out of scope for this PR? Since we just focusing on AIP-47?

I'd disagree that it is out of scope. If this gets merged right now, someone is still going to have to go and make these tests actually runnable the same way as the 19+ others which have already been converted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants