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

Restore RDS DB from snapshot #120

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Restore RDS DB from snapshot #120

wants to merge 11 commits into from

Conversation

tewfik-ghariani
Copy link
Contributor

This PR introduces the possibility of restoring an RDS DB from snapshot.
Given that the RDS DB instance restored is automatically placed in the default SG, added the support for immediately modifying it to apply the submitted VPC SG

Additionally, the RDS module has been converted to boto3 instead of boto2

Other changes

  • RDS Subnet group: fix the destroy operation
  • Example file of RDS DB in VPC
  • Blackify
  • Upgrading boto3 version and re-locking poetry

cc @grahamc, @adisbladis, @RaitoBezarius

RaitoBezarius
RaitoBezarius reviewed Nov 26, 2020
View reviewed changes
Copy link
Member

@RaitoBezarius RaitoBezarius left a comment

Choose a reason for hiding this comment

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

+1 on handling better errors and a new test, so thank you! Otherwise, it would be awesome to cut this PR into 3 PRs:

(1) Boto3 port.
(2) RDS snapshot feature.
(3) Unrelated minor changes like those on RDS subnets.

Anyway, except those points, LGTM.

nixops_aws/resources/ec2_rds_dbinstance.py
@@ -28,8 +30,8 @@ class EC2RDSDbInstanceDefinition(nixops.resources.ResourceDefinition):

config: Ec2RdsDbinstanceOptions
subnet_group: Optional[str]
vpc_security_groups: Optional[Sequence[str]] = None
rds_dbinstance_security_groups: Optional[Sequence[str]] = None
vpc_security_groups: Optional[Sequence[str]] = []
Copy link
Member

Choose a reason for hiding this comment

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

So, is it still relevant to keep Optional ? Same for the line below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is still relevant of course, the 'vpc_security_groups' can be omitted since it's optional but by default it would have the empty array value.

nixops_aws/resources/ec2_rds_dbinstance.py

self.rds_dbinstance_port = int(dbinstance.endpoint[1])
self.rds_dbinstance_endpoint = "%s:%d" % dbinstance.endpoint
self.rds_dbinstance_id = dbinstance["DBInstanceIdentifier"]
Copy link
Member

Choose a reason for hiding this comment

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

I'd have abstracted the AWS strings into a String Enum enum.Enum, rather than multiplying hardcoded references to it, that way, it's easy to change them in all the relevant places. But it can be done in a further PR IMHO.

nixops_aws/resources/rds_db_subnet_group.py
def __init__(self, name: str, config: nixops.resources.ResourceEval):
super(RDSDbSubnetGroupDefinition, self).__init__(name, config)

self.group_name: str = self.config.name
Copy link
Member

Choose a reason for hiding this comment

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

I thought it was mostly about RDS snapshot & boto 3 port, is this change really required in this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is to mainly fix the destroy operation bug related to the subnet-group resource but also to streamline the resource definition and follow the standard

nixops_aws/resources/rds_db_subnet_group.py
@@ -35,8 +49,8 @@ class RDSDbSubnetGroupState(nixops.resources.ResourceState[RDSDbSubnetGroupDefin
region = nixops.util.attr_property("region", None)
description = nixops.util.attr_property("description", None)
subnet_ids = nixops.util.attr_property("subnet_ids", [], "json")
access_key_id = nixops.util.attr_property("accessKeyId", None)
Copy link
Member

Choose a reason for hiding this comment

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

Why is it required now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was moved from line 39 to this place so that in the new line 83, we would access it from the defn directly instead of from the config

tewfik-ghariani
tewfik-ghariani commented Nov 27, 2020
View reviewed changes
Copy link
Contributor Author

@tewfik-ghariani tewfik-ghariani 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 the review @RaitoBezarius

Actually I split my commits in a way that each change would be viewed on its own to make the review easier

(1) Boto3 port. fb5ea12
(2) RDS snapshot feature. 576b701
(3) Unrelated minor changes like those on RDS subnets. :

nixops_aws/resources/ec2_rds_dbinstance.py
@@ -28,8 +30,8 @@ class EC2RDSDbInstanceDefinition(nixops.resources.ResourceDefinition):

config: Ec2RdsDbinstanceOptions
subnet_group: Optional[str]
vpc_security_groups: Optional[Sequence[str]] = None
rds_dbinstance_security_groups: Optional[Sequence[str]] = None
vpc_security_groups: Optional[Sequence[str]] = []
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is still relevant of course, the 'vpc_security_groups' can be omitted since it's optional but by default it would have the empty array value.

nixops_aws/resources/rds_db_subnet_group.py
def __init__(self, name: str, config: nixops.resources.ResourceEval):
super(RDSDbSubnetGroupDefinition, self).__init__(name, config)

self.group_name: str = self.config.name
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is to mainly fix the destroy operation bug related to the subnet-group resource but also to streamline the resource definition and follow the standard

nixops_aws/resources/rds_db_subnet_group.py
@@ -35,8 +49,8 @@ class RDSDbSubnetGroupState(nixops.resources.ResourceState[RDSDbSubnetGroupDefin
region = nixops.util.attr_property("region", None)
description = nixops.util.attr_property("description", None)
subnet_ids = nixops.util.attr_property("subnet_ids", [], "json")
access_key_id = nixops.util.attr_property("accessKeyId", None)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was moved from line 39 to this place so that in the new line 83, we would access it from the defn directly instead of from the config

@tewfik-ghariani tewfik-ghariani mentioned this pull request Oct 12, 2021
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RaitoBezarius RaitoBezarius RaitoBezarius left review comments

@manari-oumaima manari-oumaima manari-oumaima 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.
4 participants
@tewfik-ghariani @RaitoBezarius @manari-oumaima @adisbladis