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

Add AWS operators to create and delete RDS Database #24099

Merged
merged 14 commits into from
Jun 28, 2022
Merged

Add AWS operators to create and delete RDS Database #24099

merged 14 commits into from
Jun 28, 2022

Conversation

eskarimov
Copy link
Contributor

This PR intends to close #23822 by adding new AWS operators for create and delete RDS database.

@eskarimov eskarimov requested a review from mik-laj as a code owner June 2, 2022 06:59
@eskarimov eskarimov force-pushed the 23822-add-aws-operator-to-create-rds branch from c7e8c93 to 627c80b Compare June 2, 2022 12:31
potiuk
potiuk reviewed Jun 6, 2022
View reviewed changes
Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

Can you please implement examples in AIP-47 compatible way? You will find a lot of AIP-47 as examples.

@eskarimov eskarimov force-pushed the 23822-add-aws-operator-to-create-rds branch from 627c80b to bbd3156 Compare June 8, 2022 06:49
@eskarimov
Copy link
Contributor Author

Can you please implement examples in AIP-47 compatible way? You will find a lot of AIP-47 as examples.

Sure, I should have checked this epic 👍

I've just pushed a commit with according changes.

eladkal
eladkal reviewed Jun 8, 2022
View reviewed changes
tests/providers/amazon/aws/operators/test_rds.py
Comment on lines +476 to +478
rds_kwargs={
"DBName": DB_INSTANCE_NAME,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

If we have db_instance_identifier why do we need DBName in rds_kwargs?

Copy link
Contributor Author

@eskarimov eskarimov Jun 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

In case with Postgres it's different parameters - db_instance_identifier identifies the DB instance, while DBName is the name of the database to create when the DB instance is created.

We can actually omit DBName, then a database named postgres is created in the DB instance.

airflow/providers/amazon/aws/operators/rds.py Outdated Show resolved Hide resolved
airflow/providers/amazon/aws/operators/rds.py Outdated Show resolved Hide resolved
@eladkal
Copy link
Contributor

eladkal commented Jun 8, 2022

Would be nice also to fix dms example dag to use the operators added by this PRas you can see from the review in #23681 (comment)
This will help also to make sure we have everything we need covered by these operators (the feature request was created from #23681 )

@potiuk
Copy link
Member

potiuk commented Jun 11, 2022

Comments to @vincbeck comments @eskarimov ?

@eskarimov
Copy link
Contributor Author

Would be nice also to fix dms example dag to use the operators added by this PRas you can see from the review in #23681 (comment) This will help also to make sure we have everything we need covered by these operators (the feature request was created from #23681 )

I agree it'd be nice, however, maybe it'd be better to make in a separate PR together with DAG refactoring making it AIP-47 compatible, in order to keep the scope of this PR not too broad?

Comments to @vincbeck comments @eskarimov ?

Just replied, very nice feedback, I'm going to fix mentioned things in the upcoming days.

@eladkal
Copy link
Contributor

eladkal commented Jun 13, 2022

in order to keep the scope of this PR not too broad?

I think that it's not too broad.
The reason that I'm interested in changing dms example dag here as well is to see if we have everything we need covered. We might find that we are missing some parameter or we have something to discuss. It should be relatively simple.
Please try it - if it will cause difficulties then just report back and we can learn from it as well.

@eskarimov eskarimov force-pushed the 23822-add-aws-operator-to-create-rds branch from ee5b7a8 to a11b12d Compare June 17, 2022 10:21
@eskarimov
Copy link
Contributor Author

@eladkal could you review refactored example_dms DAG, please?
I've tried to split it into more granular tasks, i.e. instead of one set_up task having create_db_instance >> create_sample_table() -> create_dms_assets() (and according deletion tasks).

josh-fell
josh-fell reviewed Jun 17, 2022
View reviewed changes
airflow/providers/amazon/aws/example_dags/example_dms.py Outdated Show resolved Hide resolved
airflow/providers/amazon/aws/operators/rds.py Outdated Show resolved Hide resolved
airflow/providers/amazon/aws/operators/rds.py Outdated Show resolved Hide resolved
airflow/providers/amazon/aws/operators/rds.py Outdated Show resolved Hide resolved
@eladkal eladkal self-requested a review June 17, 2022 15:56
eladkal
eladkal approved these changes Jun 26, 2022
View reviewed changes
Copy link
Contributor

@eladkal eladkal left a comment

Choose a reason for hiding this comment

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

LGTM
@eskarimov can you please address josh comments above?

@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Jun 26, 2022
@github-actions
Copy link

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@eskarimov
Copy link
Contributor Author

eskarimov commented Jun 27, 2022 •
edited
Loading

LGTM @eskarimov can you please address josh comments above?

Sure, pushing updated and rebased branch. Thank you for the reviews, @josh-fell I'll mark the suggestions above as resolved then.

@eskarimov eskarimov force-pushed the 23822-add-aws-operator-to-create-rds branch from a11b12d to 206e884 Compare June 27, 2022 10:48
@eladkal eladkal merged commit bf72752 into apache:main Jun 28, 2022
@eskarimov eskarimov deleted the 23822-add-aws-operator-to-create-rds branch June 29, 2022 06:39
@potiuk potiuk mentioned this pull request Jul 13, 2022
Closed
95 tasks
@hankehly hankehly mentioned this pull request Aug 27, 2022
Merged
2 tasks
kaxil pushed a commit that referenced this pull request Sep 5, 2022
@hankehly @ferruzzi
Set template_fields on RDS operators (#26005)
751cb9a 
Related: #24099 #25952

### Summary

This PR sets the `template_fields` property on `RdsCreateDbInstanceOperator` and `RdsDeleteDbInstanceOperator` to allow users to programmatically change the database id, instance size, allocated storage, etc..

It also replaces use of `@task` decorated functions with their appropriate operator in system tests.


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

@josh-fell josh-fell josh-fell left review comments

@potiuk potiuk potiuk left review comments

@vincbeck vincbeck vincbeck approved these changes

@eladkal eladkal eladkal approved these changes

@mik-laj mik-laj Awaiting requested review from mik-laj

Assignees
No one assigned
Labels
area:providers full tests needed We need to run full set of tests for this PR to merge kind:documentation provider:amazon-aws AWS/Amazon - related issues
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add an AWS operator for Create RDS Database
5 participants
@eskarimov @eladkal @potiuk @josh-fell @vincbeck